Re: [RFC] cycle timer read extension for raw1394/libraw1394

Re: [RFC] cycle timer read extension for raw1394/libraw1394

[Stefan Richter]

(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/

Re: [RFC] cycle timer read extension for raw1394/libraw1394

[Pieter Palmers]

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 */
>>
>>
> 

Re: [RFC] cycle timer read extension for raw1394/libraw1394

[Stefan Richter]

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/

[PATCH update] ieee1394: cycle timer read extension for raw1394/libraw1394

[Stefan Richter]

From: Pieter Palmers <pieterp@joow.be>

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 <stefanr@s5r6.in-berlin.de>
---

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/

which header for local_irq_save? (was Re: [PATCH update] ieee1394: cycle timer read extension for raw1394/libraw1394)

[Stefan Richter]

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/

Re: [PATCH update] ieee1394: cycle timer read extension for raw1394/libraw1394

[Pieter Palmers]

Stefan Richter wrote:
> From: Pieter Palmers <pieterp@joow.be>
> 
> 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

Re: [PATCH update] ieee1394: cycle timer read extension for raw1394/libraw1394

[Stefan Richter]

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/

Re: [PATCH update] ieee1394: cycle timer read extension for raw1394/libraw1394

[Pieter Palmers]

[-- Attachment #1: Type: text/plain, Size: 1169 bytes --]

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

[-- Attachment #2: ctr_test.c --]
[-- Type: text/x-csrc, Size: 7530 bytes --]

/*   Parts of this are originally from:
 *
 *   FreeBob = Firewire (pro-)audio for linux
 *
 *   http://freebob.sf.net
 *
 *   Copyright (C) 2005,2006,2007 Pieter Palmers <pieterpalmers@users.sourceforge.net>
 *
 *   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.
 *
 */

/*
compile:
gcc -g -o ctr_test -lraw1394 ctr_test.c
*/


#include <libraw1394/raw1394.h>
#include <stdio.h>
#include <errno.h>
#include <stdint.h>
#include <unistd.h>
#include <stdlib.h>
#include <signal.h>
#include <inttypes.h>

// Some configuration constants 
#define CC_SLEEP_TIME_AFTER_DLL_UPDATE    2000
#define CC_DLL_COEFF     (0.0001)
#define CC_INIT_MAX_TRIES 10
#define USECS_BETWEEN_PRINT (1000000LLU)

// Definitions and utility macro's to handle the ISO cycle timer

#define CYCLES_PER_SECOND   8000u
#define TICKS_PER_CYCLE     3072u
#define TICKS_PER_SECOND    24576000u
#define TICKS_PER_USEC     (TICKS_PER_SECOND/1000000.0)

#define CYCLE_COUNTER_GET_SECS(x)   ((((x) & 0xFE000000) >> 25))
#define CYCLE_COUNTER_GET_CYCLES(x) ((((x) & 0x01FFF000) >> 12))
#define CYCLE_COUNTER_GET_OFFSET(x)  ((((x) & 0x00000FFF)))
#define CYCLE_COUNTER_TO_TICKS(x) ((CYCLE_COUNTER_GET_SECS(x)   * TICKS_PER_SECOND) +\
                                   (CYCLE_COUNTER_GET_CYCLES(x) * TICKS_PER_CYCLE ) +\
                                   (CYCLE_COUNTER_GET_OFFSET(x)            ))

#define CYCLE_COUNTER_UNWRAP_TICKS(x) ((x) \
                                       + (127 * TICKS_PER_SECOND) \
                                       + (CYCLES_PER_SECOND * TICKS_PER_CYCLE) \
                                       + (TICKS_PER_CYCLE) \
                                      )

// globals
uint64_t m_lastmeas_usecs;
unsigned int m_cyclecounter_ticks;
double m_ticks_per_usec;

// signal handler
int keep_running=1;

static void sighandler (int sig)
{
	keep_running = 0;
}

// DLL functions
int init_dll(uint64_t usecs1, unsigned int ticks1,
             uint64_t usecs2, unsigned int ticks2) {
    double rate=0.0;
    unsigned int delta_ticks;
    
    if (ticks2 > ticks1) {
        delta_ticks=ticks2 - ticks1;
    } else { // wraparound
        delta_ticks=CYCLE_COUNTER_UNWRAP_TICKS(ticks2) - ticks1;
    }
    
    int delta_usecs=usecs2-usecs1;
    
    rate=((double)delta_ticks/(double)delta_usecs);
    
    // update the internal values
    m_cyclecounter_ticks=ticks2;
    m_lastmeas_usecs=usecs2;

    m_ticks_per_usec=rate;
    
    printf("init rate=%6.4f\n", rate);
}

int update_dll(uint64_t new_usecs, unsigned int new_ticks) {
    uint64_t prev_usecs=m_lastmeas_usecs;
    unsigned int prev_ticks=m_cyclecounter_ticks;
    
    // the difference in system time
    int delta_usecs=new_usecs-prev_usecs;

    // the measured cycle counter difference
    long unsigned int delta_ticks_meas;
    if (new_ticks > prev_ticks) {
        delta_ticks_meas=new_ticks - prev_ticks;
    } else { // wraparound
        delta_ticks_meas=CYCLE_COUNTER_UNWRAP_TICKS(new_ticks) - prev_ticks;
    }
    
    // the estimated cycle counter difference
    unsigned int delta_ticks_est=(unsigned int)(m_ticks_per_usec * ((double)delta_usecs));
    
    // the measured & estimated rate
    double rate_meas=((double)delta_ticks_meas/(double)delta_usecs);
    double rate_est=((double)m_ticks_per_usec);
    
    int diff=(int)delta_ticks_est;
    
    // calculate the difference in predicted ticks and
    // measured ticks
    diff -= delta_ticks_meas;
    
    
    if (diff > 24000 || diff < -24000) { // approx +/-1 msec error
        printf("Bad pred: diff=%d, dt_est=%u, dt_meas=%u, d=%dus, err=%fus\n",
        diff, delta_ticks_est, delta_ticks_meas, delta_usecs, (((double)diff)/24.576)
        );
    }
    
    // calculate the error 
    double err=rate_meas-rate_est;
    
    // first order DLL update to obtain the rate.
    m_ticks_per_usec += CC_DLL_COEFF*err;
    
    // update the internal values
    m_cyclecounter_ticks += delta_ticks_est;
    // if we need to wrap, do it
    if (m_cyclecounter_ticks > TICKS_PER_SECOND * 128) {
            m_cyclecounter_ticks -= TICKS_PER_SECOND * 128;
    }

    m_lastmeas_usecs = new_usecs;

    return 0;
}

// main
int32_t main(int32_t argc, char **argv)
{

    int port=0;

    raw1394handle_t handle;
    uint64_t last_local_time=0;
    struct raw1394_cycle_timer ctr;
    struct raw1394_cycle_timer ctr2;
    int err;

    printf("libraw1394 Cycle Timer API test application\n");

    if (argc==2) {
        port = atoi(argv[1]);
    }

    printf("using port %d\n",port);

    // get handle
    handle = raw1394_new_handle_on_port(port);
    if (handle == NULL) {
        perror("raw1394_new_handle");
        return -1;
    }
    
    // register signal handler
    signal (SIGINT, sighandler);
    signal (SIGPIPE, sighandler);
    
    // init the DLL
    err=raw1394_read_cycle_timer(handle, &ctr);
    if(err) {
        perror("raw1394_read_cycle_timer");
    }

    usleep(CC_SLEEP_TIME_AFTER_DLL_UPDATE);

    err=raw1394_read_cycle_timer(handle, &ctr2);
    if(err) {
        perror("raw1394_read_cycle_timer");
    }

    init_dll(ctr.local_time,CYCLE_COUNTER_TO_TICKS(ctr.cycle_timer),
             ctr2.local_time,CYCLE_COUNTER_TO_TICKS(ctr2.cycle_timer));

    usleep(CC_SLEEP_TIME_AFTER_DLL_UPDATE);

    printf("\n");
    // start the monitor loop
    while (keep_running) {
        ctr.local_time=0;

        err=raw1394_read_cycle_timer(handle, &ctr);
        if(err) {
            perror("raw1394_read_cycle_timer");
        } else {
            uint64_t local_time_usec=ctr.local_time;
            double local_time_msec=(double)local_time_usec;
            unsigned int offset=CYCLE_COUNTER_GET_OFFSET(ctr.cycle_timer);
            unsigned int cycles=CYCLE_COUNTER_GET_CYCLES(ctr.cycle_timer);
            unsigned int secs=CYCLE_COUNTER_GET_SECS(ctr.cycle_timer);
            
            local_time_msec /= 1000.0;
            
            update_dll(ctr.local_time,CYCLE_COUNTER_TO_TICKS(ctr.cycle_timer));
            
            if ((ctr.local_time - last_local_time) > USECS_BETWEEN_PRINT) {
                const uint64_t usecs_per_day=24LLU*60LLU*60LLU*1000000LLU;
                const uint64_t usecs_per_year=356LLU*usecs_per_day;

                uint64_t years=local_time_usec/usecs_per_year;
                uint64_t days=local_time_usec%usecs_per_year;
                days /= usecs_per_day;
                
                printf("Local time: %16lluus, %16.3fms (approx %2lluyear, %3lluday since epoch)\n",
                    local_time_usec,local_time_msec, years, days);
                printf("CycleTimer: %3us, %4ucy, %4uticks\n",secs,cycles,offset);
                printf(" rate: %10.6fticks/usec\n\n",m_ticks_per_usec);

                last_local_time=ctr.local_time;
            }
        }

        usleep(CC_SLEEP_TIME_AFTER_DLL_UPDATE);
    }

    return 0;
}

Re: [PATCH update] ieee1394: cycle timer read extension for raw1394/libraw1394

[Stefan Richter]

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/

Re: [PATCH update] ieee1394: cycle timer read extension for raw1394/libraw1394

[Stefan Richter]

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/

Re: [PATCH update] ieee1394: cycle timer read extension for raw1394/libraw1394

[Pieter Palmers]

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

compat_ioctl (was [PATCH update] ieee1394: cycle timer read extension for raw1394/libraw1394)

[Stefan Richter]

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/

Re: compat_ioctl (was [PATCH update] ieee1394: cycle timer read extension for raw1394/libraw1394)

[Arnd Bergmann]

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 <><