Bug 7773

Summary: read the cycle time and system time simultaneously
Product: Drivers Reporter: Pieter Palmers (pieterp)
Component: IEEE1394Assignee: Stefan Richter (stefanr)
Status: CLOSED CODE_FIX    
Severity: normal CC: dan
Priority: P2    
Hardware: i386   
OS: Linux   
Kernel Version: 2.6.20 Subsystem:
Regression: --- Bisected commit-id:
Attachments: cycle timer read extension for raw1394
cycle timer read extension for libraw1394
cycle timer read extension for raw1394
cycle timer read extension for raw1394
cycle timer read extension for libraw1394
cycle timer read extension for libraw1394, API changed
cycle timer read extension for libraw1394

Description Pieter Palmers 2007-01-05 03:39:53 UTC
Libraw1394 at present requires an async write and read to get the cycle time. 
It would be nice if it was read directly from the kernel.

Secondly, there is currently no function that reads the cycle time and system
time simultaneously so that they are synchronized.  It would be great if such a
function existed so we could discover the exact receive time of an ISO packet
and express it as a system time with microsecond accuracy.
Comment 1 Stefan Richter 2007-02-03 02:16:16 UTC
Proposed patch posted by Pieter at
http://thread.gmane.org/gmane.linux.kernel.firewire.devel/8934
Comment 2 Stefan Richter 2007-02-03 05:00:25 UTC
Created attachment 10268 [details]
cycle timer read extension for raw1394
Comment 3 Stefan Richter 2007-02-03 05:01:33 UTC
Created attachment 10269 [details]
cycle timer read extension for libraw1394
Comment 4 Stefan Richter 2007-02-03 07:55:05 UTC
Created attachment 10270 [details]
cycle timer read extension for raw1394

Update 2:
  - Use 1000000ULL instead of USEC_PER_SEC,
  - include asm/system.h instead of linux/irqflags.h for
    local_irq_save and local_irq_restore.
Comment 5 Stefan Richter 2007-02-03 08:40:53 UTC
Created attachment 10271 [details]
cycle timer read extension for raw1394

revert struct name to Pieter's original version
Comment 6 Stefan Richter 2007-02-11 05:41:42 UTC
Created attachment 10377 [details]
cycle timer read extension for libraw1394

Update of the libraw1394 part of the patch:
  - comments,
  - fully copied the kernel's ieee1394-ioctl.h,
  - slightly shrunk main.c::raw1394_read_cycle_timer


There remains a difference between libraw1394's kernel-raw1394.h and the
kernel's raw1394.h:

--- src/kernel-raw1394.h	2007-02-11 14:16:35.000000000 +0100
+++ ../../linux-2.6.20/drivers/ieee1394/raw1394.h    2007-02-11
01:23:05.000000000 +0100
@@ -27,6 +27,7 @@
 #define RAW1394_REQ_GET_ROM	     203
 #define RAW1394_REQ_UPDATE_ROM      204
 #define RAW1394_REQ_ECHO	     205
+#define RAW1394_REQ_MODIFY_ROM      206

 #define RAW1394_REQ_ARM_REGISTER    300
 #define RAW1394_REQ_ARM_UNREGISTER  301
@@ -104,18 +105,18 @@
	 __u8		 extended_transaction_code;
	 __u32		 generation;
	 __u16		 buffer_length;
-	 __u8		 *buffer;
+	 __u8		 __user *buffer;
 } *arm_request_t;

 typedef struct arm_response {
	 __s32		 response_code;
	 __u16		 buffer_length;
-	 __u8		 *buffer;
+	 __u8		 __user *buffer;
 } *arm_response_t;

 typedef struct arm_request_response {
-	 struct arm_request  *request;
-	 struct arm_response *response;
+	 struct arm_request  __user *request;
+	 struct arm_response __user *response;
 } *arm_request_response_t;

 /* rawiso API */
@@ -135,7 +136,7 @@
 /* argument for RAW1394_ISO_RECV/XMIT_PACKETS ioctls */
 struct raw1394_iso_packets {
	__u32 n_packets;
-	struct raw1394_iso_packet_info *infos;
+	struct raw1394_iso_packet_info __user *infos;
 };

 struct raw1394_iso_config {
Comment 7 Stefan Richter 2007-02-11 05:47:36 UTC
How about changing main.c::raw1394_read_cycle_timer to:

int raw1394_read_cycle_timer(raw1394handle_t handle,
                             u_int32_t *cycle_timer
                             u_int64_t *local_time)
{
        int err;
        struct _raw1394_cycle_timer ctr = { 0 };

        err = ioctl(handle->fd, RAW1394_IOC_GET_CYCLE_TIMER, &ctr);
        if (!err) {
                *cycle_timer = ctr.cycle_timer;
                *local_time  = ctr.local_time;
        }
        return err;
}

The benefit of doing so would be that we would get rid of libraw1394's struct
raw1394_cycle_timer which duplicates the kernel's struct _raw1394_cycle_timer.
(Of course we would rename the latter to struct raw1394_cycle_timer then.)
Comment 8 Stefan Richter 2007-02-11 06:14:03 UTC
Created attachment 10378 [details]
cycle timer read extension for libraw1394, API changed

as proposed in the previous comment
Comment 9 Stefan Richter 2007-02-11 06:36:39 UTC
Created attachment 10379 [details]
cycle timer read extension for libraw1394

inserted a dropped comment again
Comment 10 Dan Dennedy 2007-02-11 22:53:33 UTC
I tested out the latest versions of the patches, and they work for me. 
However, I wonder if we should should bump the raw1394 protocol version. 
Without it and a version mismatch between raw1394 and libraw1394, one gets a 
EINVAL when calling raw1394_read_cycle_timer. By bumping the protocol version 
and a small change in libraw1394, one can get an EPROTO on raw1394_new_handle. 
What do you think? 
Comment 11 Stefan Richter 2007-02-12 05:11:07 UTC
OK, let's change RAW1394_KERNELAPI_VERSION to 5. The additional check in
libraw1394 is minimal and gives application clients a better return value. I
will commit this tonight to git and push to Linus on Saturday.
Comment 12 Dan Dennedy 2007-02-13 20:31:36 UTC
I have second thoughts on this version bump. A simple version bump in kernel 
raw1394 will cause released libraw1394 versions to succeed and use protocol 
version 3. This causes special handling resulting in libraw1394 
raw1394_get_irm_id() returning unknown values, thereby breaking 
raw1394_bandwidth_modify(), raw1394_channel_modify(), and app calls. With some 
additional change to kernel raw1394 we can remove backwards compatibilty. 
However, I think it would suck for users to have libraw1394 and its apps break 
when they upgrade to kernel 2.4.21 because there is nothing like shared 
library versioning to help reinforce dependencies.
Comment 13 Stefan Richter 2007-02-21 03:29:55 UTC
The feature was merged in mainline and is available since 2.6.21-rc1.