Possible bug: SO_TIMESTAMPING 2.6.30+

Possible bug: SO_TIMESTAMPING 2.6.30+

[Marcus D. Leech]

I've searched both the LKML archives, and the patchsets for kernels 
since 2.6.30 to see if
   this problem has been observed.

I have an Ethernet driver that supports hardware timestamping, and it 
has been working
   swimmingly well for several months using the SO_TIMESTAMPING 
infrastructure that
   Patrick Ohly put in the kernel starting around 2.6.30.

What I've discovered, that really causes me to pull my hair out, is that 
packets that are
   either AF_INET6 or AF_PACKET, *are not* getting Tx (transmit) 
timestamps.  Diving into
   the driver, I've discovered that the sk_buffs associated with these 
packets *dont* have
   the appropriate skb_tx state in the skb:

               shtx = skb_tx(skb);
               if (shtx->hardware)
               {
                        /* Do the timestamping thing */
                }

Now the puzzling thing is that the socket that originates these packets 
has SO_TIMESTAMPING
   turned on, and was created as an AF_INET6 socket, but carries both V4 
and V6 traffic.  The
   V4 traffic skbs  have  shtx->hardware  true appropriately,  but the 
V6 traffic doesn't.  Using
   Wireshark, I can clearly see the V6 packets leaving the interface, so 
it's not like they're
   getting routed somewhere else, and debugging in the driver *clearly* 
shows that V4 packets
   have shtx->hardware, while the V6 ones (from the SAME SOCKET) do not.

I know that Patrick Ohly has essentially moved on from doing the 
SO_TIMESTAMPING stuff, so
   who's maintaining it now?

When I look at the V6 udp code, it seems to call the same udp_sendmsg() 
function that V4
   uses, which appears to support Tx timestamping the way it should.

Any clues anyone?

Re: Possible bug: SO_TIMESTAMPING 2.6.30+

[Christopher Zimmermann]

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

On Mon, 09 Nov 2009 19:40:30 -0500
"Marcus D. Leech" <mleech@ripnet.com> wrote:


> I know that Patrick Ohly has essentially moved on from doing the 
> SO_TIMESTAMPING stuff, so
>    who's maintaining it now?

I worked on it a month ago or so and have a patchset from Patick Ohly
and some changes by me which fix software timestamping and make the
ioctl interface to the hardware more flexible (keeping backwards
compatibility). Patches are attached.
Still I never tried IPv6 and don't think the patches do anything about
it.
It would be nice to know weather software tx timestamps work
with/without the patches.


Christopher

[-- Attachment #2: 0001-timecompare-use-ARRAY_SIZE-macro.patch --]
[-- Type: text/x-patch, Size: 941 bytes --]

>From 5e408d962dbbafc581d4442d2e920dc667c0d078 Mon Sep 17 00:00:00 2001
From: Christopher Zimmermann <madroach@zakweb.de>
Date: Mon, 17 Aug 2009 21:26:17 +0200
Subject: [PATCH 1/3] timecompare: use ARRAY_SIZE() macro

---
 kernel/time/timecompare.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/time/timecompare.c b/kernel/time/timecompare.c
index 71e7f1a..fe24977 100644
--- a/kernel/time/timecompare.c
+++ b/kernel/time/timecompare.c
@@ -56,11 +56,11 @@ int timecompare_offset(struct timecompare *sync,
 	int index;
 	int num_samples = sync->num_samples;
 
-	if (num_samples > sizeof(buffer)/sizeof(buffer[0])) {
+	if (num_samples > ARRAY_SIZE(buffer)) {
 		samples = kmalloc(sizeof(*samples) * num_samples, GFP_ATOMIC);
 		if (!samples) {
 			samples = buffer;
-			num_samples = sizeof(buffer)/sizeof(buffer[0]);
+			num_samples = ARRAY_SIZE(buffer);
 		}
 	} else {
 		samples = buffer;
-- 
1.6.3.3


[-- Attachment #3: 0002-net-software-TX-time-stamping.patch --]
[-- Type: text/x-patch, Size: 6336 bytes --]

>From 50d8dfcf781fd3e9ad1d6123ff204b9f77a185ea Mon Sep 17 00:00:00 2001
From: Christopher Zimmermann <madroach@zakweb.de>
Date: Mon, 17 Aug 2009 21:27:50 +0200
Subject: [PATCH 2/3] net: software TX time stamping

This patch implements the software fallback to TX time stamping. The
necessary access to the buffer and socket are secured by taking
references before calling ndo_start_xmit().

That avoids race conditions (buffer remains available even if
transmission completes before ndo_start_xmit() returns) and works
even if the driver calls skb_orphan().

The caller of skb_tstamp_tx() is now responsible for providing the
socket, which requires minor changes in users of the previous call:
add skb->sk as parameter to get the old behavior.
---
 drivers/net/igb/igb_main.c |    2 +-
 include/linux/skbuff.h     |   11 +++++---
 net/core/dev.c             |   65 ++++++++++++++++++++++++++++++++++++++++++++
 net/core/skbuff.c          |    4 +-
 4 files changed, 75 insertions(+), 7 deletions(-)

diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index adb09d3..3ce4d78 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -4370,7 +4370,7 @@ static void igb_tx_hwtstamp(struct igb_adapter *adapter, struct sk_buff *skb)
 			shhwtstamps.hwtstamp = ns_to_ktime(ns);
 			shhwtstamps.syststamp =
 				timecompare_transform(&adapter->compare, ns);
-			skb_tstamp_tx(skb, &shhwtstamps);
+			skb_tstamp_tx(skb, skb->sk, &shhwtstamps);
 		}
 	}
 }
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index f2c69a2..e29e43d 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1873,17 +1873,20 @@ static inline ktime_t net_invalid_timestamp(void)
 
 /**
  * skb_tstamp_tx - queue clone of skb with send time stamps
- * @orig_skb:	the original outgoing packet
+ * @skb:	the original outgoing packet
+ * @sk:		sending socket (either from skb->sk or previous sock_get()),
+ *		may be NULL
  * @hwtstamps:	hardware time stamps, may be NULL if not available
  *
- * If the skb has a socket associated, then this function clones the
+ * If the socket is available, then this function clones the
  * skb (thus sharing the actual data and optional structures), stores
  * the optional hardware time stamping information (if non NULL) or
  * generates a software time stamp (otherwise), then queues the clone
  * to the error queue of the socket.  Errors are silently ignored.
  */
-extern void skb_tstamp_tx(struct sk_buff *orig_skb,
-			struct skb_shared_hwtstamps *hwtstamps);
+extern void skb_tstamp_tx(struct sk_buff *skb,
+			  struct sock *sk,
+			  struct skb_shared_hwtstamps *hwtstamps);
 
 extern __sum16 __skb_checksum_complete_head(struct sk_buff *skb, int len);
 extern __sum16 __skb_checksum_complete(struct sk_buff *skb);
diff --git a/net/core/dev.c b/net/core/dev.c
index 6a94475..e20e3ca 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1679,10 +1679,71 @@ static int dev_gso_segment(struct sk_buff *skb)
 	return 0;
 }
 
+/**
+ * struct tx_tstamp_context - context for software TX time stamping
+ * @sk: socket reference, NULL if nothing to do
+ * @skb: packet reference
+ */
+struct tx_tstamp_context {
+	struct sock *sk;
+	struct sk_buff *skb;
+};
+
+/**
+ * tx_tstamp_start - check for TX software time stamping and prepare for it
+ * @skb: buffer which is being sent
+ * @context: needs to be initialized for tx_tstamp_end()
+ */
+static void tx_tstamp_start(struct sk_buff *skb,
+			    struct tx_tstamp_context *context)
+{
+	union skb_shared_tx *shtx = skb_tx(skb);
+	/*
+	 * Prepare for TX time stamping in software if requested.
+	 * This could be optimized so that device drivers
+	 * do that themselves, which avoids the skb/sk ref/unref
+	 * overhead.
+	 */
+	if (unlikely(shtx->software &&
+		     skb->sk)) {
+		context->sk = skb->sk;
+		sock_hold(skb->sk);
+		context->skb = skb_get(skb);
+	} else {
+		/* TX software time stamping not requested/not possible. */
+		context->sk = NULL;
+	}
+}
+
+/**
+ * tx_tstamp_end - finish the work started by tx_tstamp_end()
+ * @context: may contain socket and buffer references
+ * @rc: result of ndo_start_xmit() - only do time stamping if packet was sent
+ */
+static void tx_tstamp_end(struct tx_tstamp_context *context, int rc)
+{
+	if (unlikely(context->sk)) {
+		union skb_shared_tx *shtx = skb_tx(context->skb);
+		/*
+		 * Checking shtx->software again is a bit redundant: it must
+		 * have been set in tx_tstamp_start(), but perhaps it
+		 * was cleared in the meantime to disable the TX software
+		 * fallback.
+		 */
+		if (likely(!rc) &&
+		    unlikely(shtx->software &&
+			     !shtx->in_progress))
+			skb_tstamp_tx(context->skb, context->sk, NULL);
+		sock_put(context->sk);
+		kfree_skb(context->skb);
+	}
+}
+
 int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
 			struct netdev_queue *txq)
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
+	struct tx_tstamp_context context;
 	int rc;
 
 	if (likely(!skb->next)) {
@@ -1703,6 +1764,7 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
 		if (dev->priv_flags & IFF_XMIT_DST_RELEASE)
 			skb_dst_drop(skb);
 
+		tx_tstamp_start(skb, &context);
 		rc = ops->ndo_start_xmit(skb, dev);
 		if (rc == 0)
 			txq_trans_update(txq);
@@ -1720,6 +1782,7 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
 		 * the skb destructor before the call and restoring it
 		 * afterwards, then doing the skb_orphan() ourselves?
 		 */
+		tx_tstamp_end(&context, rc);
 		return rc;
 	}
 
@@ -1729,7 +1792,9 @@ gso:
 
 		skb->next = nskb->next;
 		nskb->next = NULL;
+		tx_tstamp_start(skb, &context);
 		rc = ops->ndo_start_xmit(nskb, dev);
+		tx_tstamp_end(&context, rc);
 		if (unlikely(rc)) {
 			nskb->next = skb->next;
 			skb->next = nskb;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 9e0597d..78ae3e9 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2971,9 +2971,9 @@ int skb_cow_data(struct sk_buff *skb, int tailbits, struct sk_buff **trailer)
 EXPORT_SYMBOL_GPL(skb_cow_data);
 
 void skb_tstamp_tx(struct sk_buff *orig_skb,
-		struct skb_shared_hwtstamps *hwtstamps)
+		   struct sock *sk,
+		   struct skb_shared_hwtstamps *hwtstamps)
 {
-	struct sock *sk = orig_skb->sk;
 	struct sock_exterr_skb *serr;
 	struct sk_buff *skb;
 	int err;
-- 
1.6.3.3


[-- Attachment #4: 0003-Change-kernel-timestamping-interface.patch --]
[-- Type: text/x-patch, Size: 11963 bytes --]

>From fc396501b8d0906253b1f85945b86da4130ee665 Mon Sep 17 00:00:00 2001
From: Christopher Zimmermann <madroach@sancho.(none)>
Date: Wed, 7 Oct 2009 17:28:26 +0200
Subject: [PATCH 3/3] Change kernel timestamping interface

Allow userspace to fine-tune hardware registers.
Stay backwards compatible to old userspace?
---
 .gitignore                   |    1 +
 drivers/net/igb/e1000_regs.h |   41 ++++++++++--------
 drivers/net/igb/igb_main.c   |   95 +++++++++++++++++++++++++----------------
 include/linux/net_tstamp.h   |   35 +++++++++++----
 4 files changed, 107 insertions(+), 65 deletions(-)

diff --git a/.gitignore b/.gitignore
index b93fb7e..6fac88c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -32,6 +32,7 @@
 #
 # Top-level generic files
 #
+debian
 tags
 TAGS
 vmlinux
diff --git a/drivers/net/igb/e1000_regs.h b/drivers/net/igb/e1000_regs.h
index 6e59245..1785b30 100644
--- a/drivers/net/igb/e1000_regs.h
+++ b/drivers/net/igb/e1000_regs.h
@@ -83,30 +83,33 @@
 #define E1000_TSYNCRXCTL_VALID (1<<0)
 #define E1000_TSYNCRXCTL_ENABLED (1<<4)
 enum {
+	/* bits 1 to 3 need leftshift <<1 for direct register access. */
 	E1000_TSYNCRXCTL_TYPE_L2_V2 = 0,
-	E1000_TSYNCRXCTL_TYPE_L4_V1 = (1<<1),
-	E1000_TSYNCRXCTL_TYPE_L2_L4_V2 = (1<<2),
-	E1000_TSYNCRXCTL_TYPE_ALL = (1<<3),
-	E1000_TSYNCRXCTL_TYPE_EVENT_V2 = (1<<3) | (1<<1),
+	E1000_TSYNCRXCTL_TYPE_L4_V1 = (1<<0),
+	E1000_TSYNCRXCTL_TYPE_L2_L4_V2 = (1<<1),
+	E1000_TSYNCRXCTL_TYPE_ALL = (1<<2),
+	E1000_TSYNCRXCTL_TYPE_EVENT_V2 = (1<<2) | (1<<0),
 };
 #define E1000_TSYNCRXCFG 0x05F50
 enum {
-	E1000_TSYNCRXCFG_PTP_V1_SYNC_MESSAGE = 0<<0,
-	E1000_TSYNCRXCFG_PTP_V1_DELAY_REQ_MESSAGE = 1<<0,
-	E1000_TSYNCRXCFG_PTP_V1_FOLLOWUP_MESSAGE = 2<<0,
-	E1000_TSYNCRXCFG_PTP_V1_DELAY_RESP_MESSAGE = 3<<0,
-	E1000_TSYNCRXCFG_PTP_V1_MANAGEMENT_MESSAGE = 4<<0,
+	/* bits 0 to 7 */
+	E1000_TSYNCRXCFG_PTP_V1_SYNC_MESSAGE = 0,
+	E1000_TSYNCRXCFG_PTP_V1_DELAY_REQ_MESSAGE = 1,
+	E1000_TSYNCRXCFG_PTP_V1_FOLLOWUP_MESSAGE = 2,
+	E1000_TSYNCRXCFG_PTP_V1_DELAY_RESP_MESSAGE = 3,
+	E1000_TSYNCRXCFG_PTP_V1_MANAGEMENT_MESSAGE = 4,
 
-	E1000_TSYNCRXCFG_PTP_V2_SYNC_MESSAGE = 0<<8,
-	E1000_TSYNCRXCFG_PTP_V2_DELAY_REQ_MESSAGE = 1<<8,
-	E1000_TSYNCRXCFG_PTP_V2_PATH_DELAY_REQ_MESSAGE = 2<<8,
-	E1000_TSYNCRXCFG_PTP_V2_PATH_DELAY_RESP_MESSAGE = 3<<8,
-	E1000_TSYNCRXCFG_PTP_V2_FOLLOWUP_MESSAGE = 8<<8,
-	E1000_TSYNCRXCFG_PTP_V2_DELAY_RESP_MESSAGE = 9<<8,
-	E1000_TSYNCRXCFG_PTP_V2_PATH_DELAY_FOLLOWUP_MESSAGE = 0xA<<8,
-	E1000_TSYNCRXCFG_PTP_V2_ANNOUNCE_MESSAGE = 0xB<<8,
-	E1000_TSYNCRXCFG_PTP_V2_SIGNALLING_MESSAGE = 0xC<<8,
-	E1000_TSYNCRXCFG_PTP_V2_MANAGEMENT_MESSAGE = 0xD<<8,
+	/* bits 8 to 11 need leftshift <<8 for direct register access */
+	E1000_TSYNCRXCFG_PTP_V2_SYNC_MESSAGE = 0,
+	E1000_TSYNCRXCFG_PTP_V2_DELAY_REQ_MESSAGE = 1,
+	E1000_TSYNCRXCFG_PTP_V2_PATH_DELAY_REQ_MESSAGE = 2,
+	E1000_TSYNCRXCFG_PTP_V2_PATH_DELAY_RESP_MESSAGE = 3,
+	E1000_TSYNCRXCFG_PTP_V2_FOLLOWUP_MESSAGE = 8,
+	E1000_TSYNCRXCFG_PTP_V2_DELAY_RESP_MESSAGE = 9,
+	E1000_TSYNCRXCFG_PTP_V2_PATH_DELAY_FOLLOWUP_MESSAGE = 0xA,
+	E1000_TSYNCRXCFG_PTP_V2_ANNOUNCE_MESSAGE = 0xB,
+	E1000_TSYNCRXCFG_PTP_V2_SIGNALLING_MESSAGE = 0xC,
+	E1000_TSYNCRXCFG_PTP_V2_MANAGEMENT_MESSAGE = 0xD,
 };
 #define E1000_SYSTIML 0x0B600
 #define E1000_SYSTIMH 0x0B604
diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index adb09d3..17bfd5b 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -1512,7 +1512,7 @@ static int __devinit igb_probe(struct pci_dev *pdev,
 	wr32(E1000_TIMINCA,
 	     (1<<24) |
 	     IGB_TSYNC_CYCLE_TIME_IN_NANOSECONDS * IGB_TSYNC_SCALE);
-#if 0
+#if 1
 	/*
 	 * Avoid rollover while we initialize by resetting the time counter.
 	 */
@@ -4881,28 +4881,33 @@ static int igb_hwtstamp_ioctl(struct net_device *netdev,
 	struct igb_adapter *adapter = netdev_priv(netdev);
 	struct e1000_hw *hw = &adapter->hw;
 	struct hwtstamp_config config;
+#if 0
 	u32 tsync_tx_ctl_bit = E1000_TSYNCTXCTL_ENABLED;
 	u32 tsync_rx_ctl_bit = E1000_TSYNCRXCTL_ENABLED;
 	u32 tsync_rx_ctl_type = 0;
 	u32 tsync_rx_cfg = 0;
 	int is_l4 = 0;
 	int is_l2 = 0;
-	short port = 319; /* PTP */
+#endif
 	u32 regval;
 
+	memset(&config, 0, sizeof(config));
+        /* TODO: first check, weather old or new interface is passed. */
 	if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
 		return -EFAULT;
 
 	/* reserved for future extensions */
 	if (config.flags)
-		return -EINVAL;
+		goto manual;
 
 	switch (config.tx_type) {
 	case HWTSTAMP_TX_OFF:
-		tsync_tx_ctl_bit = 0;
+		//tsync_tx_ctl_bit = 0;
+		config.flags &= ~HWTSTAMP_TX;
 		break;
 	case HWTSTAMP_TX_ON:
-		tsync_tx_ctl_bit = E1000_TSYNCTXCTL_ENABLED;
+		//tsync_tx_ctl_bit = E1000_TSYNCTXCTL_ENABLED;
+		config.flags |= HWTSTAMP_TX;
 		break;
 	default:
 		return -ERANGE;
@@ -4910,7 +4915,7 @@ static int igb_hwtstamp_ioctl(struct net_device *netdev,
 
 	switch (config.rx_filter) {
 	case HWTSTAMP_FILTER_NONE:
-		tsync_rx_ctl_bit = 0;
+		config.flags &= ~HWTSTAMP_RX;
 		break;
 	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
@@ -4921,57 +4926,68 @@ static int igb_hwtstamp_ioctl(struct net_device *netdev,
 		 * possible to time stamp both Sync and Delay_Req messages
 		 * => fall back to time stamping all packets
 		 */
-		tsync_rx_ctl_type = E1000_TSYNCRXCTL_TYPE_ALL;
+		//tsync_rx_ctl_type = E1000_TSYNCRXCTL_TYPE_ALL;
+		config.igb.ptp_type = E1000_TSYNCRXCTL_TYPE_ALL;
 		config.rx_filter = HWTSTAMP_FILTER_ALL;
 		break;
 	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
-		tsync_rx_ctl_type = E1000_TSYNCRXCTL_TYPE_L4_V1;
-		tsync_rx_cfg = E1000_TSYNCRXCFG_PTP_V1_SYNC_MESSAGE;
-		is_l4 = 1;
+		config.igb.ptp_type = E1000_TSYNCRXCTL_TYPE_L4_V1;
+		config.igb.ptp1_control = E1000_TSYNCRXCFG_PTP_V1_SYNC_MESSAGE;
+		config.igb.port = htons(319); /* PTP */
 		break;
 	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
-		tsync_rx_ctl_type = E1000_TSYNCRXCTL_TYPE_L4_V1;
-		tsync_rx_cfg = E1000_TSYNCRXCFG_PTP_V1_DELAY_REQ_MESSAGE;
-		is_l4 = 1;
+		config.igb.ptp_type = E1000_TSYNCRXCTL_TYPE_L4_V1;
+		config.igb.ptp1_control = E1000_TSYNCRXCFG_PTP_V1_DELAY_REQ_MESSAGE;
+		config.igb.port = htons(319); /* PTP */
 		break;
 	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
-		tsync_rx_ctl_type = E1000_TSYNCRXCTL_TYPE_L2_L4_V2;
-		tsync_rx_cfg = E1000_TSYNCRXCFG_PTP_V2_SYNC_MESSAGE;
-		is_l2 = 1;
-		is_l4 = 1;
+		config.igb.ptp_type = E1000_TSYNCRXCTL_TYPE_L2_L4_V2;
+		config.igb.ptp2_id = E1000_TSYNCRXCFG_PTP_V2_SYNC_MESSAGE;
+		config.igb.etype = 0x88F7;
+		config.igb.port = htons(319); /* PTP */
 		config.rx_filter = HWTSTAMP_FILTER_SOME;
 		break;
 	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
 	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
-		tsync_rx_ctl_type = E1000_TSYNCRXCTL_TYPE_L2_L4_V2;
-		tsync_rx_cfg = E1000_TSYNCRXCFG_PTP_V2_DELAY_REQ_MESSAGE;
-		is_l2 = 1;
-		is_l4 = 1;
+		config.igb.ptp_type = E1000_TSYNCRXCTL_TYPE_L2_L4_V2;
+		config.igb.ptp2_id = E1000_TSYNCRXCFG_PTP_V2_DELAY_REQ_MESSAGE;
+		config.igb.etype = 0x88F7;
+		config.igb.port = htons(319); /* PTP */
 		config.rx_filter = HWTSTAMP_FILTER_SOME;
 		break;
 	case HWTSTAMP_FILTER_PTP_V2_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
-		tsync_rx_ctl_type = E1000_TSYNCRXCTL_TYPE_EVENT_V2;
+		config.igb.ptp_type = E1000_TSYNCRXCTL_TYPE_EVENT_V2;
 		config.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
-		is_l2 = 1;
+		config.igb.etype = 0x88F7;
 		break;
 	default:
 		return -ERANGE;
 	}
 
+manual:
 	/* enable/disable TX */
 	regval = rd32(E1000_TSYNCTXCTL);
-	regval = (regval & ~E1000_TSYNCTXCTL_ENABLED) | tsync_tx_ctl_bit;
+	if(config.flags & HWTSTAMP_TX)
+		regval = (regval | E1000_TSYNCTXCTL_ENABLED);
+	else	regval = (regval & ~E1000_TSYNCTXCTL_ENABLED);
 	wr32(E1000_TSYNCTXCTL, regval);
 
 	/* enable/disable RX, define which PTP packets are time stamped */
 	regval = rd32(E1000_TSYNCRXCTL);
-	regval = (regval & ~E1000_TSYNCRXCTL_ENABLED) | tsync_rx_ctl_bit;
-	regval = (regval & ~0xE) | tsync_rx_ctl_type;
+	if(config.flags & HWTSTAMP_RX)
+		regval = (regval | E1000_TSYNCRXCTL_ENABLED);
+	else	regval = (regval & ~E1000_TSYNCRXCTL_ENABLED);
+	regval = (regval & ~0xE) | config.igb.ptp_type << 1;
 	wr32(E1000_TSYNCRXCTL, regval);
-	wr32(E1000_TSYNCRXCFG, tsync_rx_cfg);
+
+	/* we don't need to read since we can reset the whole register. */
+	regval =  config.igb.ptp1_control
+		| config.igb.ptp2_id << 8
+		| config.igb.ptp2_val << 12;
+	wr32(E1000_TSYNCRXCFG, regval);
 
 	/*
 	 * Ethertype Filter Queue Filter[0][15:0] = 0x88F7
@@ -4979,27 +4995,32 @@ static int igb_hwtstamp_ioctl(struct net_device *netdev,
 	 * Ethertype Filter Queue Filter[0][26] = 0x1 (Enable filter)
 	 * Ethertype Filter Queue Filter[0][30] = 0x1 (Enable Timestamping)
 	 */
-	wr32(E1000_ETQF0, is_l2 ? 0x440088f7 : 0);
+	wr32(E1000_ETQF0, config.igb.etype ? 0x4400 << 16 | config.igb.etype : 0);
 
 	/* L4 Queue Filter[0]: only filter by source and destination port */
-	wr32(E1000_SPQF0, htons(port));
-	wr32(E1000_IMIREXT(0), is_l4 ?
+	/* wr32(E1000_SPQF0, port); XXX why source port? */
+	wr32(E1000_IMIREXT(0), config.igb.port ?
 	     ((1<<12) | (1<<19) /* bypass size and control flags */) : 0);
-	wr32(E1000_IMIR(0), is_l4 ?
-	     (htons(port)
+	wr32(E1000_IMIR(0), config.igb.port ?
+	     (config.igb.port
 	      | (0<<16) /* immediate interrupt disabled */
-	      | 0 /* (1<<17) bit cleared: do not bypass
+	      | (0<<17) /* (1<<17) bit cleared: do not bypass
 		     destination port check */)
 		: 0);
-	wr32(E1000_FTQF0, is_l4 ?
+	wr32(E1000_FTQF0, config.igb.port ?
 	     (0x11 /* UDP */
 	      | (1<<15) /* VF not compared */
 	      | (1<<27) /* Enable Timestamping */
+#if 0
 	      | (7<<28) /* only source port filter enabled,
 			   source/target address and protocol
-			   masked */)
-	     : ((1<<15) | (15<<28) /* all mask bits set = filter not
-				      enabled */));
+			   masked */
+#endif
+	      | (0xe<<28) /* only protocol filter enabled
+			   * everything else is masked. */
+	     )
+	     : (1<<15) | (15<<28) /* all mask bits set = filter not
+				      enabled */);
 
 	wrfl();
 
diff --git a/include/linux/net_tstamp.h b/include/linux/net_tstamp.h
index a3b8546..7b069dd 100644
--- a/include/linux/net_tstamp.h
+++ b/include/linux/net_tstamp.h
@@ -40,26 +40,43 @@ enum {
  */
 struct hwtstamp_config {
 	int flags;
-	int tx_type;
-	int rx_filter;
+	/* deprecated, only used for backwards compatibility when flags == 0 */
+	int tx_type;            /* put this into flags ? */
+	int rx_filter;          /* deprecated */
+	/* hardware dependend part starts here */
+	union {
+		struct {
+			unsigned short port;  /* for dst port filter, zero to disable */
+			unsigned short etype; /* for ethertype filter, zero to disable */
+			unsigned ptp1_control	:8; /* TSYNCRXCFG.CTRLT */
+			/* Careful! According to reference manual message ids
+			 * 2 and 3 always get timestamped. */
+			unsigned ptp2_id	:4; /* TSYNCRXCFG.MSGT */
+			unsigned ptp2_val	:4; /* TSYNCRXCFG.TRNSSPC */
+			unsigned ptp_type	:3; /* TSYNCRXCTL.Type */
+		} igb;
+	};
 };
 
-/* possible values for hwtstamp_config->tx_type */
+/* flags for hwtstamp_config */
 enum {
 	/*
-	 * No outgoing packet will need hardware time stamping;
-	 * should a packet arrive which asks for it, no hardware
-	 * time stamping will be done.
+	 * these first two values are the only allowed values in
+	 * deprecated hwtstamp_config->tx_type
 	 */
-	HWTSTAMP_TX_OFF,
-
 	/*
 	 * Enables hardware time stamping for outgoing packets;
 	 * the sender of the packet decides which are to be
 	 * time stamped by setting %SOF_TIMESTAMPING_TX_SOFTWARE
 	 * before sending the packet.
+	 * If disabled no packet will be timestamped; even if it
+	 * asks for it.
 	 */
-	HWTSTAMP_TX_ON,
+	HWTSTAMP_TX_OFF = 0, /* only used in hwtstamp_config->tx_type */
+	HWTSTAMP_TX_ON = 1,
+	HWTSTAMP_TX = 1,
+
+	HWTSTAMP_RX = 2
 };
 
 /* possible values for hwtstamp_config->rx_filter */
-- 
1.6.3.3

Re: Possible bug: SO_TIMESTAMPING 2.6.30+

[Eric Dumazet]

Christopher Zimmermann a écrit :
> On Mon, 09 Nov 2009 19:40:30 -0500
> "Marcus D. Leech" <mleech@ripnet.com> wrote:
> 
> 
>> I know that Patrick Ohly has essentially moved on from doing the 
>> SO_TIMESTAMPING stuff, so
>>    who's maintaining it now?
> 
> I worked on it a month ago or so and have a patchset from Patick Ohly
> and some changes by me which fix software timestamping and make the
> ioctl interface to the hardware more flexible (keeping backwards
> compatibility). Patches are attached.
> Still I never tried IPv6 and don't think the patches do anything about
> it.
> It would be nice to know weather software tx timestamps work
> with/without the patches.
> 
> 
> Christopher
> 

I see some sock_put()/sock_hold() stuff in your second patch.

We removed these things on transmit path, please dont add them back...

If skb->sk is set, it probably also has a destructor and a reference already taken.

(This reference being on sk_wmem_alloc by the way, not on sk_refcnt)

You probably can try to change skb destructor ?

Re: Possible bug: SO_TIMESTAMPING 2.6.30+

[Christopher Zimmermann]

On Tue, 10 Nov 2009 09:12:52 +0100
Christopher Zimmermann <madroach@zakweb.de> wrote:

> On Mon, 09 Nov 2009 19:40:30 -0500
> "Marcus D. Leech" <mleech@ripnet.com> wrote:
> 
> 
> > I know that Patrick Ohly has essentially moved on from doing the 
> > SO_TIMESTAMPING stuff, so
> >    who's maintaining it now?
> 
> I worked on it a month ago or so and have a patchset from Patick Ohly
> and some changes by me which fix software timestamping and make the
> ioctl interface to the hardware more flexible (keeping backwards
> compatibility). Patches are attached.
> Still I never tried IPv6 and don't think the patches do anything about
> it.
> It would be nice to know weather software tx timestamps work
> with/without the patches.
> 
> 
> Christopher

Have a look at commit 51f31cabe3ce5345b51e4a4f82138b38c4d5dc91.
It adds support for SO_TIMESTAMPING in the transport layer, but only
for the ipv4 variants.
Good news is, it does look like it was simple to do the same to
ipv6/udp.c. Have fun.


    ip: support for TX timestamps on UDP and RAW sockets
    
    Instructions for time stamping outgoing packets are take from the
    socket layer and later copied into the new skb.
    
    Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>


But only in ipv4/udp.c:

-------------------------------- net/ipv4/udp.c
-------------------------------- index c47c989..4bd178a 100644
@@ -596,6 +596,7 @@ int udp_sendmsg(struct kiocb *iocb, struct sock
*sk, struct msghdr *msg, return -EOPNOTSUPP;
 
 	ipc.opt = NULL;
+	ipc.shtx.flags = 0;
 
 	if (up->pending) {
 		/*
@@ -643,6 +644,9 @@ int udp_sendmsg(struct kiocb *iocb, struct sock
*sk, struct msghdr *msg, ipc.addr = inet->saddr;
 
 	ipc.oif = sk->sk_bound_dev_if;
+	err = sock_tx_timestamp(msg, sk, &ipc.shtx);
+	if (err)
+		return err;
 	if (msg->msg_controllen) {
 		err = ip_cmsg_send(sock_net(sk), msg, &ipc);
 		if (err)

Re: Possible bug: SO_TIMESTAMPING 2.6.30+

[Marcus D. Leech]

Christopher Zimmermann wrote:
>
>
> -------------------------------- net/ipv4/udp.c
> -------------------------------- index c47c989..4bd178a 100644
> @@ -596,6 +596,7 @@ int udp_sendmsg(struct kiocb *iocb, struct sock
> *sk, struct msghdr *msg, return -EOPNOTSUPP;
>  
>  	ipc.opt = NULL;
> +	ipc.shtx.flags = 0;
>  
>  	if (up->pending) {
>  		/*
> @@ -643,6 +644,9 @@ int udp_sendmsg(struct kiocb *iocb, struct sock
> *sk, struct msghdr *msg, ipc.addr = inet->saddr;
>  
>  	ipc.oif = sk->sk_bound_dev_if;
> +	err = sock_tx_timestamp(msg, sk, &ipc.shtx);
> +	if (err)
> +		return err;
>  	if (msg->msg_controllen) {
>  		err = ip_cmsg_send(sock_net(sk), msg, &ipc);
>  		if (err)
>
>   
That is already in 2.6.30

I took a look this afternoon at ipv6/udp.c and ipv6/ip6_output.c   and
was able to glue-in a small (a few lines) piece of code
  that apparently makes TX timestamping work for V6 DATAGRAM sockets--I
had to do it in ipv6/ip6_output.c, otherwise,
  I'd have to modify calls to the v6 _append function to pass in an
extra parameter to indicate hardware Tx timestamping.
  It was easier to simply pick up shtx->hardware in the _append
function, and then check to see if sk->sk_type == SOCK_DGRAM,
  and then set the appropriate fields in the skb.  In ipv4/udp.c, this
is done in udp_sendmsg(), before calling the _append function.

Doing this in AF_PACKET looked like it was going to be *very* easy, but
in my tests so far, it doesn't do anything useful.  More
  investigation in the lab this evening, and then I'll put together some
cdiffs against 2.6.32-rc5.  Once that's done, perhaps I can get
  some guidance about how to post those patches?

The same kind of stuff doesn't appear to work for AF_PACKET, so once I
have AF_PACKET working, I'll perhaps post a
  patch set.  I'm very new to this community, so I don't know how to go
about posting patches "officially" etc.  But it seems that
  getting this working for at least:

     V4  SOCK_DGRAM
     V6  SOCK_DGRAM
     AF_PACKET SOCK_DGRAM/SOCK_RAW

Should be quite straightforward--using the existing code in ipv4/udp.c 
as a guide.   Hopefully, if I can get this going for all 3, I'll get
  to keep my job :-)   [just kidding!].


Cheers
Marcus
currently a IEEE 1588-2008 code monkey at a semiconductor company

-- 
Marcus Leech
Principal Investigator, Shirleys Bay Radio Astronomy Consortium
http://www.sbrac.org