(Cc lkml)
Pieter Palmers wrote to linux1394-devel:
> Attached are two patches containing an extension to libraw1394 and the
> kernel stack, implementing the simultaneous read of the isochronous
> cycle timer and the system clock (in usecs). This implements what has
> been requested before on the list.
>
> There is one issue left, namely that I didn't disable preemption when
> doing the timer reads. In order to have a reliable simultaneous read of
> both values, I think IRQ's and preemption should be disabled while
> reading them.
Thus?
preempt_disable();
local_irq_save(flags);
read_cycle_timer;
read_time_of_day;
local_irq_restore(flags);
preempt_enable();
in hpsb_read_cycle_timer.
> Kernel patch applies against 2.6.20-rc6.
> libraw1394 patch applies against SVN.
>
> Please comment.
>
> Greets,
>
> Pieter Palmers
>
>
> ------------------------------------------------------------------------
>
> diff -u ../linux-2.6/drivers/ieee1394/ieee1394_core.c ieee1394-2.6.20/ieee1394_core.c
> --- ../linux-2.6/drivers/ieee1394/ieee1394_core.c 2007-01-26 18:29:07.000000000 +0100
> +++ ieee1394-2.6.20/ieee1394_core.c 2007-01-26 18:37:16.000000000 +0100
> @@ -34,6 +34,8 @@
> #include <linux/suspend.h>
> #include <linux/kthread.h>
>
> +#include <linux/time.h>
> +
> #include <asm/byteorder.h>
>
> #include "ieee1394_types.h"
No empty line necessary above #include <linux/time.h>.
> @@ -940,6 +942,13 @@
> }
> }
>
> +void hpsb_read_cycle_timer(struct hpsb_host *host, u32 *ctr, u64 *local_time)
> +{
> + struct timeval tv;
Empty line would be OK here.
> + *ctr = host->driver->devctl(host, GET_CYCLE_COUNTER, 0);
> + do_gettimeofday(&tv);
> + *local_time=tv.tv_sec*1000000+tv.tv_usec;
> +}
Couldn't this overflow?
*local_time = tv.tv_sec * 1000000L + tv.tv_usec;
or
*local_time = tv.tv_sec * USEC_PER_SEC + tv.tv_usec;
> static void abort_requests(struct hpsb_host *host)
> {
> @@ -1209,6 +1218,7 @@
> EXPORT_SYMBOL(hpsb_read);
> EXPORT_SYMBOL(hpsb_write);
> EXPORT_SYMBOL(hpsb_packet_success);
> +EXPORT_SYMBOL(hpsb_read_cycle_timer);
>
> /** highlevel.c **/
> EXPORT_SYMBOL(hpsb_register_highlevel);
> diff -u ../linux-2.6/drivers/ieee1394/ieee1394_core.h ieee1394-2.6.20/ieee1394_core.h
> --- ../linux-2.6/drivers/ieee1394/ieee1394_core.h 2007-01-26 18:11:17.000000000 +0100
> +++ ieee1394-2.6.20/ieee1394_core.h 2007-01-26 18:37:16.000000000 +0100
> @@ -227,4 +227,8 @@
> extern struct class hpsb_host_class;
> extern struct class *hpsb_protocol_class;
>
> +/* Reads the cycle timer, and the local time at which it was read.
> + */
> +void hpsb_read_cycle_timer(struct hpsb_host *host, u32 *ctr, u64 *local_time);
> +
> #endif /* _IEEE1394_CORE_H */
(This reminds me that many of ieee1394's exported symbols lack kerneldoc
comments.)
Alas I can't comment on the design & implementation of the userspace
ABI, that's out of my league.
> diff -u ../linux-2.6/drivers/ieee1394/ieee1394-ioctl.h ieee1394-2.6.20/ieee1394-ioctl.h
> --- ../linux-2.6/drivers/ieee1394/ieee1394-ioctl.h 2007-01-26 18:11:17.000000000 +0100
> +++ ieee1394-2.6.20/ieee1394-ioctl.h 2007-01-26 18:40:04.000000000 +0100
> @@ -100,5 +100,6 @@
> _IO ('#', 0x28)
> #define RAW1394_IOC_ISO_RECV_FLUSH \
> _IO ('#', 0x29)
> -
> +#define RAW1394_IOC_GET_CYCLE_TIMER \
> + _IOR ('#', 0x30, struct _raw1394_cycle_timer)
> #endif /* __IEEE1394_IOCTL_H */
> diff -u ../linux-2.6/drivers/ieee1394/raw1394.c ieee1394-2.6.20/raw1394.c
> --- ../linux-2.6/drivers/ieee1394/raw1394.c 2007-01-26 18:11:17.000000000 +0100
> +++ ieee1394-2.6.20/raw1394.c 2007-01-26 18:37:16.000000000 +0100
> @@ -2675,7 +2675,22 @@
> return dma_region_mmap(&fi->iso_handle->data_buf, file, vma);
> }
>
> -/* ioctl is only used for rawiso operations */
> +/* read the Isochronous Cycle Counter along with the cpu time */
> +static int raw1394_read_cycle_timer(struct file_info *fi, void __user * uaddr)
> +{
> + struct _raw1394_cycle_timer ctr;
> + struct hpsb_host *host = fi->host;
> +
> + hpsb_read_cycle_timer(host, &ctr.cycle_timer, &ctr.local_time);
> + if (copy_to_user(uaddr, &ctr, sizeof(ctr)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +/* ioctl is only used for rawiso operations,
> + * and to read the cycle timer
> + */
> static int raw1394_ioctl(struct inode *inode, struct file *file,
> unsigned int cmd, unsigned long arg)
> {
> @@ -2772,6 +2787,13 @@
> break;
> }
>
> + switch(cmd) {
> + case RAW1394_IOC_GET_CYCLE_TIMER:
> + return raw1394_read_cycle_timer(fi, argp);
> + default:
> + break;
> + }
> +
> return -EINVAL;
> }
>
> diff -u ../linux-2.6/drivers/ieee1394/raw1394.h ieee1394-2.6.20/raw1394.h
> --- ../linux-2.6/drivers/ieee1394/raw1394.h 2007-01-26 13:15:58.000000000 +0100
> +++ ieee1394-2.6.20/raw1394.h 2007-01-26 18:37:16.000000000 +0100
> @@ -178,4 +178,11 @@
> __s16 xmit_cycle;
> };
>
> +/* Simultanous Isochronous Cycle Timer read and local clock read */
> +struct _raw1394_cycle_timer {
> + __u32 cycle_timer;
> +
> + /* local time in microseconds */
> + __u64 local_time;
> +};
> #endif /* IEEE1394_RAW1394_H */
>
>
> ------------------------------------------------------------------------
>
> Index: src/ieee1394-ioctl.h
> ===================================================================
> --- src/ieee1394-ioctl.h (revision 168)
> +++ src/ieee1394-ioctl.h (working copy)
> @@ -106,6 +106,6 @@
> _IO ('#', 0x28)
> #define RAW1394_IOC_ISO_RECV_FLUSH \
> _IO ('#', 0x29)
> -
> -
> +#define RAW1394_IOC_GET_CYCLE_TIMER \
> + _IOR ('#', 0x30, struct _raw1394_cycle_timer)
> #endif /* __IEEE1394_IOCTL_H */
> Index: src/raw1394.h
> ===================================================================
> --- src/raw1394.h (revision 168)
> +++ src/raw1394.h (working copy)
> @@ -118,7 +118,14 @@
> RAW1394_MODIFY_FREE
> };
>
> +/* Simultanous Isochronous Cycle Timer read and local clock read */
> +struct raw1394_cycle_timer {
> + u_int32_t cycle_timer;
>
> + /* local time in microseconds */
> + u_int64_t local_time;
> +};
> +
> #ifdef __cplusplus
> extern "C" {
> #endif
> @@ -328,6 +335,18 @@
> **/
> void raw1394_iso_shutdown(raw1394handle_t handle);
>
> +/**
> + * raw1394_get_cycle_timer - get the current value of the cycle timer
> + * @handle: libraw1394 handle
> + * @ctr: pointer to the target raw1394_cycle_timer struct
> + *
> + * Reads the cycle timer register, together with the system clock. It returns
> + * a reasonable estimate of the system time the cycle timer was read.
> + *
> + * Returns: the error code of the ioctl, or 0 if successful.
> + **/
The comment on accuracy could be dropped if preemption and IRQs are
disabled, right?
> +int raw1394_read_cycle_timer(raw1394handle_t handle, struct raw1394_cycle_timer *ctr);
> +
> typedef int raw1394_errcode_t;
> #define raw1394_make_errcode(ack, rcode) (((ack) << 16) | rcode)
> #define raw1394_internal_err(errcode) ((errcode) < 0)
> Index: src/main.c
> ===================================================================
> --- src/main.c (revision 168)
> +++ src/main.c (working copy)
> @@ -478,3 +478,15 @@
> return 0;
> }
>
> +int raw1394_read_cycle_timer(raw1394handle_t handle, struct raw1394_cycle_timer *ctr)
> +{
> + int err;
> +
> + err = ioctl(handle->fd, RAW1394_IOC_GET_CYCLE_TIMER, ctr);
> + if(err != 0) {
> + return err;
> + }
> +
> + return 0;
> +}
> +
> Index: src/kernel-raw1394.h
> ===================================================================
> --- src/kernel-raw1394.h (revision 168)
> +++ src/kernel-raw1394.h (working copy)
> @@ -177,4 +177,11 @@
> __s16 xmit_cycle;
> };
>
> +/* Simultanous Isochronous Cycle Timer read and local clock read */
> +struct _raw1394_cycle_timer {
> + __u32 cycle_timer;
> +
> + /* local time in microseconds */
> + __u64 local_time;
> +};
> #endif /* IEEE1394_RAW1394_H */
>
>
--
Stefan Richter
-=====-=-=== ---= ==-==
http://arcgraph.de/sr/
Stefan Richter wrote:
> (Cc lkml)
>
> Pieter Palmers wrote to linux1394-devel:
>> Attached are two patches containing an extension to libraw1394 and the
>> kernel stack, implementing the simultaneous read of the isochronous
>> cycle timer and the system clock (in usecs). This implements what has
>> been requested before on the list.
>>
>> There is one issue left, namely that I didn't disable preemption when
>> doing the timer reads. In order to have a reliable simultaneous read of
>> both values, I think IRQ's and preemption should be disabled while
>> reading them.
>
> Thus?
> preempt_disable();
> local_irq_save(flags);
>
> read_cycle_timer;
> read_time_of_day;
>
> local_irq_restore(flags);
> preempt_enable();
>
> in hpsb_read_cycle_timer.
My main issue was that I still had to figure out how to do this... I'm a
very occasional kernel space programmer. Thanks for the hint, I'll do
it like this.
I still have to check if I don't introduce a too long non-preemptible
path though.
>
>> Kernel patch applies against 2.6.20-rc6.
>> libraw1394 patch applies against SVN.
>>
>> Please comment.
>>
>> Greets,
>>
>> Pieter Palmers
>>
>>
>> ------------------------------------------------------------------------
>>
>> diff -u ../linux-2.6/drivers/ieee1394/ieee1394_core.c ieee1394-2.6.20/ieee1394_core.c
>> --- ../linux-2.6/drivers/ieee1394/ieee1394_core.c 2007-01-26 18:29:07.000000000 +0100
>> +++ ieee1394-2.6.20/ieee1394_core.c 2007-01-26 18:37:16.000000000 +0100
>> @@ -34,6 +34,8 @@
>> #include <linux/suspend.h>
>> #include <linux/kthread.h>
>>
>> +#include <linux/time.h>
>> +
>> #include <asm/byteorder.h>
>>
>> #include "ieee1394_types.h"
>
> No empty line necessary above #include <linux/time.h>.
>
>> @@ -940,6 +942,13 @@
>> }
>> }
>>
>> +void hpsb_read_cycle_timer(struct hpsb_host *host, u32 *ctr, u64 *local_time)
>> +{
>> + struct timeval tv;
>
> Empty line would be OK here.
>
>> + *ctr = host->driver->devctl(host, GET_CYCLE_COUNTER, 0);
>> + do_gettimeofday(&tv);
>> + *local_time=tv.tv_sec*1000000+tv.tv_usec;
>> +}
>
> Couldn't this overflow?
> *local_time = tv.tv_sec * 1000000L + tv.tv_usec;
> or
> *local_time = tv.tv_sec * USEC_PER_SEC + tv.tv_usec;
Indeed. thx.
>
>> static void abort_requests(struct hpsb_host *host)
>> {
>> @@ -1209,6 +1218,7 @@
>> EXPORT_SYMBOL(hpsb_read);
>> EXPORT_SYMBOL(hpsb_write);
>> EXPORT_SYMBOL(hpsb_packet_success);
>> +EXPORT_SYMBOL(hpsb_read_cycle_timer);
>>
>> /** highlevel.c **/
>> EXPORT_SYMBOL(hpsb_register_highlevel);
>> diff -u ../linux-2.6/drivers/ieee1394/ieee1394_core.h ieee1394-2.6.20/ieee1394_core.h
>> --- ../linux-2.6/drivers/ieee1394/ieee1394_core.h 2007-01-26 18:11:17.000000000 +0100
>> +++ ieee1394-2.6.20/ieee1394_core.h 2007-01-26 18:37:16.000000000 +0100
>> @@ -227,4 +227,8 @@
>> extern struct class hpsb_host_class;
>> extern struct class *hpsb_protocol_class;
>>
>> +/* Reads the cycle timer, and the local time at which it was read.
>> + */
>> +void hpsb_read_cycle_timer(struct hpsb_host *host, u32 *ctr, u64 *local_time);
>> +
>> #endif /* _IEEE1394_CORE_H */
>
> (This reminds me that many of ieee1394's exported symbols lack kerneldoc
> comments.)
I'll change this one into proper kerneldoc format.
>
> Alas I can't comment on the design & implementation of the userspace
> ABI, that's out of my league.
>
>> diff -u ../linux-2.6/drivers/ieee1394/ieee1394-ioctl.h ieee1394-2.6.20/ieee1394-ioctl.h
>> --- ../linux-2.6/drivers/ieee1394/ieee1394-ioctl.h 2007-01-26 18:11:17.000000000 +0100
>> +++ ieee1394-2.6.20/ieee1394-ioctl.h 2007-01-26 18:40:04.000000000 +0100
>> @@ -100,5 +100,6 @@
>> _IO ('#', 0x28)
>> #define RAW1394_IOC_ISO_RECV_FLUSH \
>> _IO ('#', 0x29)
>> -
>> +#define RAW1394_IOC_GET_CYCLE_TIMER \
>> + _IOR ('#', 0x30, struct _raw1394_cycle_timer)
>> #endif /* __IEEE1394_IOCTL_H */
>> diff -u ../linux-2.6/drivers/ieee1394/raw1394.c ieee1394-2.6.20/raw1394.c
>> --- ../linux-2.6/drivers/ieee1394/raw1394.c 2007-01-26 18:11:17.000000000 +0100
>> +++ ieee1394-2.6.20/raw1394.c 2007-01-26 18:37:16.000000000 +0100
>> @@ -2675,7 +2675,22 @@
>> return dma_region_mmap(&fi->iso_handle->data_buf, file, vma);
>> }
>>
>> -/* ioctl is only used for rawiso operations */
>> +/* read the Isochronous Cycle Counter along with the cpu time */
>> +static int raw1394_read_cycle_timer(struct file_info *fi, void __user * uaddr)
>> +{
>> + struct _raw1394_cycle_timer ctr;
>> + struct hpsb_host *host = fi->host;
>> +
>> + hpsb_read_cycle_timer(host, &ctr.cycle_timer, &ctr.local_time);
>> + if (copy_to_user(uaddr, &ctr, sizeof(ctr)))
>> + return -EFAULT;
>> +
>> + return 0;
>> +}
>> +
>> +/* ioctl is only used for rawiso operations,
>> + * and to read the cycle timer
>> + */
>> static int raw1394_ioctl(struct inode *inode, struct file *file,
>> unsigned int cmd, unsigned long arg)
>> {
>> @@ -2772,6 +2787,13 @@
>> break;
>> }
>>
>> + switch(cmd) {
>> + case RAW1394_IOC_GET_CYCLE_TIMER:
>> + return raw1394_read_cycle_timer(fi, argp);
>> + default:
>> + break;
>> + }
>> +
>> return -EINVAL;
>> }
>>
>> diff -u ../linux-2.6/drivers/ieee1394/raw1394.h ieee1394-2.6.20/raw1394.h
>> --- ../linux-2.6/drivers/ieee1394/raw1394.h 2007-01-26 13:15:58.000000000 +0100
>> +++ ieee1394-2.6.20/raw1394.h 2007-01-26 18:37:16.000000000 +0100
>> @@ -178,4 +178,11 @@
>> __s16 xmit_cycle;
>> };
>>
>> +/* Simultanous Isochronous Cycle Timer read and local clock read */
>> +struct _raw1394_cycle_timer {
>> + __u32 cycle_timer;
>> +
>> + /* local time in microseconds */
>> + __u64 local_time;
>> +};
>> #endif /* IEEE1394_RAW1394_H */
>>
>>
>> ------------------------------------------------------------------------
>>
>> Index: src/ieee1394-ioctl.h
>> ===================================================================
>> --- src/ieee1394-ioctl.h (revision 168)
>> +++ src/ieee1394-ioctl.h (working copy)
>> @@ -106,6 +106,6 @@
>> _IO ('#', 0x28)
>> #define RAW1394_IOC_ISO_RECV_FLUSH \
>> _IO ('#', 0x29)
>> -
>> -
>> +#define RAW1394_IOC_GET_CYCLE_TIMER \
>> + _IOR ('#', 0x30, struct _raw1394_cycle_timer)
>> #endif /* __IEEE1394_IOCTL_H */
>> Index: src/raw1394.h
>> ===================================================================
>> --- src/raw1394.h (revision 168)
>> +++ src/raw1394.h (working copy)
>> @@ -118,7 +118,14 @@
>> RAW1394_MODIFY_FREE
>> };
>>
>> +/* Simultanous Isochronous Cycle Timer read and local clock read */
>> +struct raw1394_cycle_timer {
>> + u_int32_t cycle_timer;
>>
>> + /* local time in microseconds */
>> + u_int64_t local_time;
>> +};
>> +
>> #ifdef __cplusplus
>> extern "C" {
>> #endif
>> @@ -328,6 +335,18 @@
>> **/
>> void raw1394_iso_shutdown(raw1394handle_t handle);
>>
>> +/**
>> + * raw1394_get_cycle_timer - get the current value of the cycle timer
>> + * @handle: libraw1394 handle
>> + * @ctr: pointer to the target raw1394_cycle_timer struct
>> + *
>> + * Reads the cycle timer register, together with the system clock. It returns
>> + * a reasonable estimate of the system time the cycle timer was read.
>> + *
>> + * Returns: the error code of the ioctl, or 0 if successful.
>> + **/
>
> The comment on accuracy could be dropped if preemption and IRQs are
> disabled, right?
>
>> +int raw1394_read_cycle_timer(raw1394handle_t handle, struct raw1394_cycle_timer *ctr);
>> +
>> typedef int raw1394_errcode_t;
>> #define raw1394_make_errcode(ack, rcode) (((ack) << 16) | rcode)
>> #define raw1394_internal_err(errcode) ((errcode) < 0)
>> Index: src/main.c
>> ===================================================================
>> --- src/main.c (revision 168)
>> +++ src/main.c (working copy)
>> @@ -478,3 +478,15 @@
>> return 0;
>> }
>>
>> +int raw1394_read_cycle_timer(raw1394handle_t handle, struct raw1394_cycle_timer *ctr)
>> +{
>> + int err;
>> +
>> + err = ioctl(handle->fd, RAW1394_IOC_GET_CYCLE_TIMER, ctr);
>> + if(err != 0) {
>> + return err;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> Index: src/kernel-raw1394.h
>> ===================================================================
>> --- src/kernel-raw1394.h (revision 168)
>> +++ src/kernel-raw1394.h (working copy)
>> @@ -177,4 +177,11 @@
>> __s16 xmit_cycle;
>> };
>>
>> +/* Simultanous Isochronous Cycle Timer read and local clock read */
>> +struct _raw1394_cycle_timer {
>> + __u32 cycle_timer;
>> +
>> + /* local time in microseconds */
>> + __u64 local_time;
>> +};
>> #endif /* IEEE1394_RAW1394_H */
>>
>>
>
Pieter Palmers wrote:
> Stefan Richter wrote:
>> Thus?
>> preempt_disable();
>> local_irq_save(flags);
>>
>> read_cycle_timer;
>> read_time_of_day;
>>
>> local_irq_restore(flags);
>> preempt_enable();
>>
>> in hpsb_read_cycle_timer.
> My main issue was that I still had to figure out how to do this... I'm a
> very occasional kernel space programmer.
Unfortunately me too, and the level of ieee1394 driver maintenance shows it.
> Thanks for the hint, I'll do it like this.
>
> I still have to check if I don't introduce a too long non-preemptible
> path though.
It's a simple MMIO read (readl) and the do_gettimeofday, which should be
fine.
--
Stefan Richter
-=====-=-=== ---= ==-==
http://arcgraph.de/sr/
From: Pieter Palmers <[email protected]>
This implements the simultaneous read of the isochronous cycle timer and
the system clock (in usecs). This allows to express the exact receive
time of an ISO packet as a system time with microsecond accuracy.
http://bugzilla.kernel.org/show_bug.cgi?id=7773
The counterpart patch for libraw1394 can be found at
http://thread.gmane.org/gmane.linux.kernel.firewire.devel/8934
Patch update (Stefan R.):
- Disable preemption and local interrupts.
- Fix integer overflow.
- Add paranoid error checks and kerneldoc to hpsb_read_cycle_timer.
Move it to other ieee1394_core high-level API functions.
- Rename userspace-exported struct _raw1394_cycle_timer to
raw1394_cycle_timer. Change comments in raw1394.
- Adjust whitespace.
Signed-off-by: Stefan Richter <[email protected]>
---
Pieter, Dan, is this OK? I only compile-tested.
drivers/ieee1394/ieee1394-ioctl.h | 2 +
drivers/ieee1394/ieee1394_core.c | 43 ++++++++++++++++++++++++++++++
drivers/ieee1394/ieee1394_core.h | 3 ++
drivers/ieee1394/raw1394.c | 20 +++++++++++++
drivers/ieee1394/raw1394.h | 10 ++++++
5 files changed, 78 insertions(+)
Index: linux/drivers/ieee1394/ieee1394-ioctl.h
===================================================================
--- linux.orig/drivers/ieee1394/ieee1394-ioctl.h 2007-01-01 01:53:20.000000000 +0100
+++ linux/drivers/ieee1394/ieee1394-ioctl.h 2007-02-03 13:47:33.000000000 +0100
@@ -100,5 +100,7 @@
_IO ('#', 0x28)
#define RAW1394_IOC_ISO_RECV_FLUSH \
_IO ('#', 0x29)
+#define RAW1394_IOC_GET_CYCLE_TIMER \
+ _IOR ('#', 0x30, struct raw1394_cycle_timer)
#endif /* __IEEE1394_IOCTL_H */
Index: linux/drivers/ieee1394/ieee1394_core.c
===================================================================
--- linux.orig/drivers/ieee1394/ieee1394_core.c 2007-01-27 14:07:00.000000000 +0100
+++ linux/drivers/ieee1394/ieee1394_core.c 2007-02-03 13:47:33.000000000 +0100
@@ -33,6 +33,9 @@
#include <linux/skbuff.h>
#include <linux/suspend.h>
#include <linux/kthread.h>
+#include <linux/irqflags.h>
+#include <linux/preempt.h>
+#include <linux/time.h>
#include <asm/byteorder.h>
@@ -186,6 +189,45 @@ int hpsb_reset_bus(struct hpsb_host *hos
}
}
+/**
+ * hpsb_read_cycle_timer - read cycle timer register and system time
+ * @host: host whose isochronous cycle timer register is read
+ * @cycle_timer: address of bitfield to return the register contents
+ * @local_time: address to return the system time
+ *
+ * The format of * @cycle_timer, is described in OHCI 1.1 clause 5.13. This
+ * format is also read from non-OHCI controllers. * @local_time contains the
+ * system time in microseconds since the Epoch, read at the moment when the
+ * cycle timer was read.
+ *
+ * Return value: 0 for success or error number otherwise.
+ */
+int hpsb_read_cycle_timer(struct hpsb_host *host, u32 *cycle_timer,
+ u64 *local_time)
+{
+ int ctr;
+ struct timeval tv;
+ unsigned long flags;
+
+ if (!host || !cycle_timer || !local_time)
+ return -EINVAL;
+
+ preempt_disable();
+ local_irq_save(flags);
+
+ ctr = host->driver->devctl(host, GET_CYCLE_COUNTER, 0);
+ if (ctr)
+ do_gettimeofday(&tv);
+
+ local_irq_restore(flags);
+ preempt_enable();
+
+ if (!ctr)
+ return -EIO;
+ *cycle_timer = ctr;
+ *local_time = tv.tv_sec * USEC_PER_SEC + tv.tv_usec;
+ return 0;
+}
int hpsb_bus_reset(struct hpsb_host *host)
{
@@ -1190,6 +1232,7 @@ EXPORT_SYMBOL(hpsb_alloc_packet);
EXPORT_SYMBOL(hpsb_free_packet);
EXPORT_SYMBOL(hpsb_send_packet);
EXPORT_SYMBOL(hpsb_reset_bus);
+EXPORT_SYMBOL(hpsb_read_cycle_timer);
EXPORT_SYMBOL(hpsb_bus_reset);
EXPORT_SYMBOL(hpsb_selfid_received);
EXPORT_SYMBOL(hpsb_selfid_complete);
Index: linux/drivers/ieee1394/ieee1394_core.h
===================================================================
--- linux.orig/drivers/ieee1394/ieee1394_core.h 2007-01-01 01:53:20.000000000 +0100
+++ linux/drivers/ieee1394/ieee1394_core.h 2007-02-03 13:47:33.000000000 +0100
@@ -127,6 +127,9 @@ int hpsb_send_packet_and_wait(struct hps
* progress, 0 otherwise. */
int hpsb_reset_bus(struct hpsb_host *host, int type);
+int hpsb_read_cycle_timer(struct hpsb_host *host, u32 *cycle_timer,
+ u64 *local_time);
+
/*
* The following functions are exported for host driver module usage. All of
* them are safe to use in interrupt contexts, although some are quite
Index: linux/drivers/ieee1394/raw1394.c
===================================================================
--- linux.orig/drivers/ieee1394/raw1394.c 2007-01-27 14:07:00.000000000 +0100
+++ linux/drivers/ieee1394/raw1394.c 2007-02-03 13:47:34.000000000 +0100
@@ -2664,6 +2664,18 @@ static void raw1394_iso_shutdown(struct
fi->iso_state = RAW1394_ISO_INACTIVE;
}
+static int raw1394_read_cycle_timer(struct file_info *fi, void __user * uaddr)
+{
+ struct raw1394_cycle_timer ct;
+ int err;
+
+ err = hpsb_read_cycle_timer(fi->host, &ct.cycle_timer, &ct.local_time);
+ if (!err)
+ if (copy_to_user(uaddr, &ct, sizeof(ct)))
+ err = -EFAULT;
+ return err;
+}
+
/* mmap the rawiso xmit/recv buffer */
static int raw1394_mmap(struct file *file, struct vm_area_struct *vma)
{
@@ -2772,6 +2784,14 @@ static int raw1394_ioctl(struct inode *i
break;
}
+ /* state-independent commands */
+ switch(cmd) {
+ case RAW1394_IOC_GET_CYCLE_TIMER:
+ return raw1394_read_cycle_timer(fi, argp);
+ default:
+ break;
+ }
+
return -EINVAL;
}
Index: linux/drivers/ieee1394/raw1394.h
===================================================================
--- linux.orig/drivers/ieee1394/raw1394.h 2007-01-01 01:53:20.000000000 +0100
+++ linux/drivers/ieee1394/raw1394.h 2007-02-03 13:47:34.000000000 +0100
@@ -178,4 +178,14 @@ struct raw1394_iso_status {
__s16 xmit_cycle;
};
+/* argument to RAW1394_IOC_GET_CYCLE_TIMER ioctl */
+struct raw1394_cycle_timer {
+ /* contents of Isochronous Cycle Timer register,
+ as in OHCI 1.1 clause 5.13 (also with non-OHCI hosts) */
+ __u32 cycle_timer;
+
+ /* local time in microseconds since Epoch,
+ simultaneously read with cycle timer */
+ __u64 local_time;
+};
#endif /* IEEE1394_RAW1394_H */
--
Stefan Richter
-=====-=-=== --=- ---==
http://arcgraph.de/sr/
I wrote:
> --- linux.orig/drivers/ieee1394/ieee1394_core.c 2007-01-27 14:07:00.000000000 +0100
> +++ linux/drivers/ieee1394/ieee1394_core.c 2007-02-03 13:47:33.000000000 +0100
> @@ -33,6 +33,9 @@
> #include <linux/skbuff.h>
> #include <linux/suspend.h>
> #include <linux/kthread.h>
> +#include <linux/irqflags.h>
> +#include <linux/preempt.h>
> +#include <linux/time.h>
>
> #include <asm/byteorder.h>
>
Is <linux/irqflags.h> the correct header for local_irq_save/
local_irq_restore? It seems to be <asm/system.h> under Linux 2.6.16 but
I'm not sure about later kernels.
--
Stefan Richter
-=====-=-=== --=- ---==
http://arcgraph.de/sr/
Stefan Richter wrote:
> From: Pieter Palmers <[email protected]>
>
> This implements the simultaneous read of the isochronous cycle timer and
> the system clock (in usecs). This allows to express the exact receive
> time of an ISO packet as a system time with microsecond accuracy.
> http://bugzilla.kernel.org/show_bug.cgi?id=7773
>
> The counterpart patch for libraw1394 can be found at
> http://thread.gmane.org/gmane.linux.kernel.firewire.devel/8934
>
> Patch update (Stefan R.):
> - Disable preemption and local interrupts.
> - Fix integer overflow.
I had to use 1000000ULL instead of USEC_PER_SEC to avoid weird behavior.
> - Add paranoid error checks and kerneldoc to hpsb_read_cycle_timer.
> Move it to other ieee1394_core high-level API functions.
> - Rename userspace-exported struct _raw1394_cycle_timer to
> raw1394_cycle_timer. Change comments in raw1394.
> - Adjust whitespace.
Thanks for the cleanups!
I can't test it right now, but I'll report later.
Pieter
Pieter Palmers wrote:
> Stefan Richter wrote:
...
>> - Fix integer overflow.
> I had to use 1000000ULL instead of USEC_PER_SEC to avoid weird behavior.
OK, I'll change that and will wait for...
> I can't test it right now, but I'll report later.
...your and Dan's ACK before I commit the patch.
--
Stefan Richter
-=====-=-=== --=- ---==
http://arcgraph.de/sr/
I wrote:
> - Rename userspace-exported struct _raw1394_cycle_timer to
> raw1394_cycle_timer.
Argh. This makes maintenance of libraw1394's header files more
difficult. I will revert this to _raw1394_cycle_timer, unless somebody
has a better idea how to deal with this issue.
--
Stefan Richter
-=====-=-=== --=- ---==
http://arcgraph.de/sr/
I wrote:
> +++ linux/drivers/ieee1394/raw1394.h 2007-02-03 13:47:34.000000000 +0100
> @@ -178,4 +178,14 @@ struct raw1394_iso_status {
> __s16 xmit_cycle;
> };
>
> +/* argument to RAW1394_IOC_GET_CYCLE_TIMER ioctl */
> +struct raw1394_cycle_timer {
> + /* contents of Isochronous Cycle Timer register,
> + as in OHCI 1.1 clause 5.13 (also with non-OHCI hosts) */
> + __u32 cycle_timer;
> +
> + /* local time in microseconds since Epoch,
> + simultaneously read with cycle timer */
> + __u64 local_time;
> +};
> #endif /* IEEE1394_RAW1394_H */
Pieter,
one more thing. Do you want to hand out the cycle_timer bitfield to
userspace as-is, or would it make sense to postprocess it in some way?
--
Stefan Richter
-=====-=-=== --=- ---==
http://arcgraph.de/sr/
Stefan Richter wrote:
> I wrote:
>> +++ linux/drivers/ieee1394/raw1394.h 2007-02-03 13:47:34.000000000 +0100
>> @@ -178,4 +178,14 @@ struct raw1394_iso_status {
>> __s16 xmit_cycle;
>> };
>>
>> +/* argument to RAW1394_IOC_GET_CYCLE_TIMER ioctl */
>> +struct raw1394_cycle_timer {
>> + /* contents of Isochronous Cycle Timer register,
>> + as in OHCI 1.1 clause 5.13 (also with non-OHCI hosts) */
>> + __u32 cycle_timer;
>> +
>> + /* local time in microseconds since Epoch,
>> + simultaneously read with cycle timer */
>> + __u64 local_time;
>> +};
>> #endif /* IEEE1394_RAW1394_H */
>
> Pieter,
> one more thing. Do you want to hand out the cycle_timer bitfield to
> userspace as-is, or would it make sense to postprocess it in some way?
I like it as is. most of the time I convert it into ticks, but sometimes
I just need the cycles.
Another reason not to postprocess is that it gives userspace more
freedom in how accurate they want to use the cycle time. I'm probably
going to throw away the seconds field altogether, because one second is
a huge timeframe in my application. Throwing away the seconds field
saves me quite some calculations.
Pieter
Stefan Richter wrote:
> Pieter Palmers wrote:
>> Stefan Richter wrote:
> ...
>>> - Fix integer overflow.
>> I had to use 1000000ULL instead of USEC_PER_SEC to avoid weird behavior.
>
> OK, I'll change that and will wait for...
>
>> I can't test it right now, but I'll report later.
>
> ...your and Dan's ACK before I commit the patch.
Stefan,
I tested the patches as posted on bugzilla, and it looks like it is
working fine.
I attached a test program I used while writing/debugging the patches.
Might be useful for other people to test if this works correctly.
Compile:
$ gcc -g -o ctr_test -lraw1394 ctr_test.c
Run:
$ ./ctr_test
libraw1394 Cycle Timer API test application
using port 0
init rate=24.5837
Local time: 1170593509294313us, 1170593509294.313ms (approx 38year,
20day since epoch)
CycleTimer: 67s, 2323cy, 1894ticks
rate: 24.583702ticks/usec
Local time: 1170593510294462us, 1170593510294.462ms (approx 38year,
20day since epoch)
CycleTimer: 68s, 2328cy, 2044ticks
rate: 24.587107ticks/usec
The rate should be something around 24.576.
Since epoch is somewhere around 1970, the approx 38 years looks rather
correct.
Greets,
Pieter
I wrote on 2007-02-03:
> +#define RAW1394_IOC_GET_CYCLE_TIMER \
> + _IOR ('#', 0x30, struct raw1394_cycle_timer)
...
> +/* argument to RAW1394_IOC_GET_CYCLE_TIMER ioctl */
> +struct raw1394_cycle_timer {
> + /* contents of Isochronous Cycle Timer register,
> + as in OHCI 1.1 clause 5.13 (also with non-OHCI hosts) */
> + __u32 cycle_timer;
> +
> + /* local time in microseconds since Epoch,
> + simultaneously read with cycle timer */
> + __u64 local_time;
> +};
> #endif /* IEEE1394_RAW1394_H */
Hmm, is this struct padded on 64bit platforms?
If so, would
struct raw1394_cycle_timer {
__u64 local_time;
__u32 cycle_timer;
};
be padded too?
--
Stefan Richter
-=====-=-=== --=- -=-=-
http://arcgraph.de/sr/
On Saturday 10 February 2007 15:20, Stefan Richter wrote:
>
> I wrote on 2007-02-03:
> > +#define RAW1394_IOC_GET_CYCLE_TIMER??????????\
> > +?????_IOR ('#', 0x30, struct raw1394_cycle_timer)
> ...
> > +/* argument to RAW1394_IOC_GET_CYCLE_TIMER ioctl */
> > +struct raw1394_cycle_timer {
> > +?????/* contents of Isochronous Cycle Timer register,
> > +????? ? as in OHCI 1.1 clause 5.13 (also with non-OHCI hosts) */
> > +?????__u32 cycle_timer;
> > +
> > +?????/* local time in microseconds since Epoch,
> > +????? ? simultaneously read with cycle timer */
> > +?????__u64 local_time;
> > +};
> > ?#endif /* IEEE1394_RAW1394_H */
>
> Hmm, is this struct padded on 64bit platforms?
> If so, would
> ????????struct raw1394_cycle_timer {
> ????????????????__u64 local_time;
> ????????????????__u32 cycle_timer;
> ????????};
> be padded too?
If one of these two is padded on a given CPU, the other one is as well,
because the structure alignment is defined as the maximum alignment
of one of its members (this is needed so that arrays work).
Both are padded on all 64 bit architectures that Linux runs on, but
on x86, you get the extra twist that the 32 bit version does not
have padding. To express the right structure for compat mode, you
need something like
#ifdef __x86_64__
typedef u64 compat_u64 __attribute__((packed, aligned(4)));
#else
typedef u64 compat_u64;
#endif
struct raw1394_cycle_timer {
compat_u64 local_time;
u32 cycle_timer;
};
Arnd <><