LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Stefan Richter <stefanr@s5r6.in-berlin.de>
To: Pieter Palmers <pieterp@joow.be>
Cc: linux1394-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org
Subject: Re: [RFC] cycle timer read extension for raw1394/libraw1394
Date: Sat, 27 Jan 2007 11:21:35 +0100	[thread overview]
Message-ID: <45BB27AF.7030007@s5r6.in-berlin.de> (raw)
In-Reply-To: <45BA5CFD.6070900@joow.be>

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

       reply	other threads:[~2007-01-27 10:22 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <45BA5CFD.6070900@joow.be>
2007-01-27 10:21 ` Stefan Richter [this message]
2007-01-27 10:45   ` Pieter Palmers
2007-01-27 12:48     ` Stefan Richter
2007-02-03 12:58       ` [PATCH update] ieee1394: " Stefan Richter
2007-02-03 13:42         ` which header for local_irq_save? (was Re: [PATCH update] ieee1394: cycle timer read extension for raw1394/libraw1394) Stefan Richter
2007-02-03 14:22         ` [PATCH update] ieee1394: cycle timer read extension for raw1394/libraw1394 Pieter Palmers
2007-02-03 15:32           ` Stefan Richter
2007-02-04 12:55             ` Pieter Palmers
2007-02-03 16:42         ` Stefan Richter
2007-02-03 19:03         ` Stefan Richter
2007-02-03 20:18           ` Pieter Palmers
2007-02-10 14:20         ` compat_ioctl (was [PATCH update] ieee1394: cycle timer read extension for raw1394/libraw1394) Stefan Richter
2007-02-10 15:47           ` Arnd Bergmann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=45BB27AF.7030007@s5r6.in-berlin.de \
    --to=stefanr@s5r6.in-berlin.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux1394-devel@lists.sourceforge.net \
    --cc=pieterp@joow.be \
    --subject='Re: [RFC] cycle timer read extension for raw1394/libraw1394' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).