[PATCH 00/11] net: expand time stamping, batch #2

This patch series represents a continuation of the effort to get
better coverage of the SO_TIMESTAMPING socket API in the Ethernet
drivers.  Adding time stamping support to a given driver solves two
separate issues, namely software transmit time stamping and hardware
time stamping in PHY devices.

This second batch adds the hooks into all remaining (?) arm and
powerpc MAC drivers using phylib. The first patch exports the receive
hook for use by non-NAPI drivers when compiled as modules. Patch #5
has been tested on real hardware, but the rest have only been compile
tested.

Richard Cochran (11):
  net: export the receive time stamping hook for non-NAPI drivers
  lib8390: enable transmit and receive time stamping.
  emaclite: enable transmit and receive time stamping.
  ll_temac: enable transmit and receive time stamping.
  fec_mpc52xx: enable transmit and receive time stamping.
  macb: enable transmit time stamping.
  fs_enet: enable transmit time stamping.
  smsc911x: enable transmit time stamping.
  pxa168_eth: enable transmit time stamping.
  mv643xx_eth: enable transmit time stamping.
  ucc_geth: enable transmit time stamping.

 drivers/net/fec_mpc52xx.c          |    4 +++-
 drivers/net/fs_enet/fs_enet-main.c |    2 ++
 drivers/net/lib8390.c              |    5 +++--
 drivers/net/ll_temac_main.c        |    5 ++++-
 drivers/net/macb.c                 |    2 ++
 drivers/net/mv643xx_eth.c          |    2 ++
 drivers/net/pxa168_eth.c           |    1 +
 drivers/net/smsc911x.c             |    1 +
 drivers/net/ucc_geth.c             |    1 +
 drivers/net/xilinx_emaclite.c      |    9 +++++++--
 net/core/timestamping.c            |    1 +
 11 files changed, 27 insertions(+), 6 deletions(-)

Ethernet MAC drivers based on phylib (but not using NAPI) can
enable hardware time stamping in phy devices by calling netif_rx()
conditionally based on a call to skb_defer_rx_timestamp().

This commit exports that function so that drivers calling it may
be compiled as modules.

Signed-off-by: Richard Cochran <redacted>
---
 net/core/timestamping.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/core/timestamping.c b/net/core/timestamping.c
index 3b00a6b..98a5264 100644
--- a/net/core/timestamping.c
+++ b/net/core/timestamping.c

@@ -122,6 +122,7 @@ bool skb_defer_rx_timestamp(struct sk_buff *skb)
 
 	return false;
 }
+EXPORT_SYMBOL_GPL(skb_defer_rx_timestamp);
 
 void __init skb_timestamping_init(void)
 {

-- 
1.7.0.4

This patch enables software (and phy device) time stamping. This file is
included by drivers/net/ax88796.c, which is based on phylib. So, this
patch makes hardware time stamping in the PHY possible.

Compile tested only.

Signed-off-by: Richard Cochran <redacted>
---
 drivers/net/lib8390.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/lib8390.c b/drivers/net/lib8390.c
index 17b75e5..70eb207 100644
--- a/drivers/net/lib8390.c
+++ b/drivers/net/lib8390.c

@@ -410,7 +410,7 @@ static netdev_tx_t __ei_start_xmit(struct sk_buff *skb,
 
 	spin_unlock(&ei_local->page_lock);
 	enable_irq_lockdep_irqrestore(dev->irq, &flags);
-
+	skb_tx_timestamp(skb);
 	dev_kfree_skb (skb);
 	dev->stats.tx_bytes += send_length;
 

@@ -758,7 +758,8 @@ static void ei_receive(struct net_device *dev)
 				skb_put(skb, pkt_len);	/* Make room */
 				ei_block_input(dev, pkt_len, skb, current_offset + sizeof(rx_frame));
 				skb->protocol=eth_type_trans(skb,dev);
-				netif_rx(skb);
+				if (!skb_defer_rx_timestamp(skb))
+					netif_rx(skb);
 				dev->stats.rx_packets++;
 				dev->stats.rx_bytes += pkt_len;
 				if (pkt_stat & ENRSR_PHY)

-- 
1.7.0.4

This patch enables software (and phy device) time stamping. Since this
MAC uses phylib, adding the hooks make hardware time stamping in the phy
possible.

Compile tested only.

Cc: John Linn <redacted>
Signed-off-by: Richard Cochran <redacted>
---
 drivers/net/xilinx_emaclite.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/xilinx_emaclite.c b/drivers/net/xilinx_emaclite.c
index 372572c..ae029d0 100644
--- a/drivers/net/xilinx_emaclite.c
+++ b/drivers/net/xilinx_emaclite.c

@@ -647,7 +647,8 @@ static void xemaclite_rx_handler(struct net_device *dev)
 	dev->stats.rx_packets++;
 	dev->stats.rx_bytes += len;
 
-	netif_rx(skb);		/* Send the packet upstream */
+	if (!skb_defer_rx_timestamp(skb))
+		netif_rx(skb);	/* Send the packet upstream */
 }
 
 /**

@@ -1029,15 +1030,19 @@ static int xemaclite_send(struct sk_buff *orig_skb, struct net_device *dev)
 	spin_lock_irqsave(&lp->reset_lock, flags);
 	if (xemaclite_send_data(lp, (u8 *) new_skb->data, len) != 0) {
 		/* If the Emaclite Tx buffer is busy, stop the Tx queue and
-		 * defer the skb for transmission at a later point when the
+		 * defer the skb for transmission during the ISR, after the
 		 * current transmission is complete */
 		netif_stop_queue(dev);
 		lp->deferred_skb = new_skb;
 		spin_unlock_irqrestore(&lp->reset_lock, flags);
+		/* Take the time stamp now, since we can't do this in an ISR. */
+		skb_tx_timestamp(new_skb);
 		return 0;
 	}
 	spin_unlock_irqrestore(&lp->reset_lock, flags);
 
+	skb_tx_timestamp(new_skb);
+
 	dev->stats.tx_bytes += len;
 	dev_kfree_skb(new_skb);
 

-- 
1.7.0.4

Le dimanche 19 juin 2011 à 13:19 +0200, Richard Cochran a écrit :

Since you unlock reset_lock right here, there is no guarantee something
wont eat 'deferred_skb', so you might manipulate a freed skb after this
point.

+		/* Take the time stamp now, since we can't do this in an ISR. */
+		skb_tx_timestamp(new_skb);
 		return 0;
 	}
 	spin_unlock_irqrestore(&lp->reset_lock, flags);
 
+	skb_tx_timestamp(new_skb);
+

this one is ok.

 	dev->stats.tx_bytes += len;
 	dev_kfree_skb(new_skb);
 

This patch enables software (and phy device) time stamping. Since this MAC
is based on phylib, adding the hooks makes hardware time stamping in the
phy possible.

Compile tested only.

Cc: Grant Likely <redacted>
Signed-off-by: Richard Cochran <redacted>
---
 drivers/net/ll_temac_main.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ll_temac_main.c b/drivers/net/ll_temac_main.c
index b7948cc..eabc378 100644
--- a/drivers/net/ll_temac_main.c
+++ b/drivers/net/ll_temac_main.c

@@ -730,6 +730,8 @@ static int temac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	/* Kick off the transfer */
 	lp->dma_out(lp, TX_TAILDESC_PTR, tail_p); /* DMA start */
 
+	skb_tx_timestamp(skb);
+
 	return NETDEV_TX_OK;
 }
 

@@ -772,7 +774,8 @@ static void ll_temac_recv(struct net_device *ndev)
 			skb->ip_summed = CHECKSUM_COMPLETE;
 		}
 
-		netif_rx(skb);
+		if (!skb_defer_rx_timestamp(skb))
+			netif_rx(skb);
 
 		ndev->stats.rx_packets++;
 		ndev->stats.rx_bytes += length;

-- 
1.7.0.4

This patch enables software (and phy device) time stamping. Software
time stamping using the SO_TIMESTAMPING API was tested and found to be
working on the LITE5200B board.

Cc: Grant Likely <redacted>
Signed-off-by: Richard Cochran <redacted>
---
 drivers/net/fec_mpc52xx.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/fec_mpc52xx.c b/drivers/net/fec_mpc52xx.c
index 9f81b1a..102bdbc 100644
--- a/drivers/net/fec_mpc52xx.c
+++ b/drivers/net/fec_mpc52xx.c

@@ -337,6 +337,7 @@ static int mpc52xx_fec_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	bcom_submit_next_buffer(priv->tx_dmatsk, skb);
 	spin_unlock_irqrestore(&priv->lock, flags);
+	skb_tx_timestamp(skb);
 
 	if (bcom_queue_full(priv->tx_dmatsk)) {
 		netif_stop_queue(dev);

@@ -434,7 +435,8 @@ static irqreturn_t mpc52xx_fec_rx_interrupt(int irq, void *dev_id)
 		length = status & BCOM_FEC_RX_BD_LEN_MASK;
 		skb_put(rskb, length - 4);	/* length without CRC32 */
 		rskb->protocol = eth_type_trans(rskb, dev);
-		netif_rx(rskb);
+		if (!skb_defer_rx_timestamp(skb))
+			netif_rx(rskb);
 
 		spin_lock(&priv->lock);
 	}

-- 
1.7.0.4

Le dimanche 19 juin 2011 à 13:20 +0200, Richard Cochran a écrit :

same problem here : You should call skb_tx_timestamp() before the lock
release, or an interrupt might already freed this skb.

This patch enables software (and phy device) transmit time stamping
Compile tested only.

Cc: Nicolas Ferre <redacted>
Signed-off-by: Richard Cochran <redacted>
---
 drivers/net/macb.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/macb.c b/drivers/net/macb.c
index 6c6a028..6944478 100644
--- a/drivers/net/macb.c
+++ b/drivers/net/macb.c

@@ -676,6 +676,8 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	spin_unlock_irqrestore(&bp->lock, flags);
 
+	skb_tx_timestamp(skb);
+
 	return NETDEV_TX_OK;
 }
 

-- 
1.7.0.4

Le dimanche 19 juin 2011 à 13:20 +0200, Richard Cochran a écrit :

Same problem here, its not safe.

This patch enables software (and phy device) transmit time stamping.
Compile tested only.

Cc: Pantelis Antoniou <pantelis.antoniou@gmail.com>
Cc: Vitaly Bordug <redacted>
Signed-off-by: Richard Cochran <redacted>
---
 drivers/net/fs_enet/fs_enet-main.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_enet-main.c
index 21abb5c..8dcb272 100644
--- a/drivers/net/fs_enet/fs_enet-main.c
+++ b/drivers/net/fs_enet/fs_enet-main.c

@@ -701,6 +701,8 @@ static int fs_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	spin_unlock_irqrestore(&fep->tx_lock, flags);
 
+	skb_tx_timestamp(skb);
+
 	return NETDEV_TX_OK;
 }
 

-- 
1.7.0.4

Le dimanche 19 juin 2011 à 13:20 +0200, Richard Cochran a écrit :

Well, I'll stop my review here, there is the same problem I guess in all
your patches.

On Sun, Jun 19, 2011 at 01:58:04PM +0200, Eric Dumazet wrote:

Well, I'll stop my review here, there is the same problem I guess in all
your patches.

Thanks for your review. I have posted a fix for the first batch (since
they are already in next) and reposted this series.

But, considering your point, it looks like pxa168_eth and mv643xx_eth
(see patches 9 and 10 of this series) already access skb->len unsafely.

Would you care to comment on those spots, too?

Thanks,
Richard

Le dimanche 19 juin 2011 à 20:12 +0200, Richard Cochran a écrit :

Thanks for your review. I have posted a fix for the first batch (since
they are already in next) and reposted this series.

But, considering your point, it looks like pxa168_eth and mv643xx_eth
(see patches 9 and 10 of this series) already access skb->len unsafely.

Would you care to comment on those spots, too?

They certainly are buggy, at a first glance.

Not only skb->len is unsafe, but netif_tx_stop_queue() calls are unsafe
too.

Not sure anyone still use these drivers...

On Sun, Jun 19, 2011 at 08:30:49PM +0200, Eric Dumazet wrote:

Le dimanche 19 juin 2011 à 20:12 +0200, Richard Cochran a écrit :

quoted

Thanks for your review. I have posted a fix for the first batch (since
they are already in next) and reposted this series.

But, considering your point, it looks like pxa168_eth and mv643xx_eth
(see patches 9 and 10 of this series) already access skb->len unsafely.

Would you care to comment on those spots, too?

They certainly are buggy, at a first glance.

Not only skb->len is unsafe, but netif_tx_stop_queue() calls are unsafe
too.

Out of the MAC drivers in my two batches, only drivers/net/tg3.c calls
netif_tx_stop_queue(txq);

However, I don't know how to fix that. Anyone else care to take a look?

Thanks,

Richard

On Sun, Jun 19, 2011 at 11:58:31PM -0700, Richard Cochran wrote:

On Sun, Jun 19, 2011 at 08:30:49PM +0200, Eric Dumazet wrote:

quoted

Le dimanche 19 juin 2011 ? 20:12 +0200, Richard Cochran a ?crit :

quoted

Thanks for your review. I have posted a fix for the first batch (since
they are already in next) and reposted this series.

But, considering your point, it looks like pxa168_eth and mv643xx_eth
(see patches 9 and 10 of this series) already access skb->len unsafely.

Would you care to comment on those spots, too?

They certainly are buggy, at a first glance.

Not only skb->len is unsafe, but netif_tx_stop_queue() calls are unsafe
too.

Out of the MAC drivers in my two batches, only drivers/net/tg3.c calls
netif_tx_stop_queue(txq);

However, I don't know how to fix that. Anyone else care to take a look?

How is netif_tx_stop_queue() unsafe?

This patch enables software (and phy device) transmit time stamping.
Compile tested only.

Cc: Steve Glendinning <redacted>
Signed-off-by: Richard Cochran <redacted>
---
 drivers/net/smsc911x.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/smsc911x.c b/drivers/net/smsc911x.c
index c6d47d1..6d08b0f 100644
--- a/drivers/net/smsc911x.c
+++ b/drivers/net/smsc911x.c

@@ -1473,6 +1473,7 @@ static int smsc911x_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	pdata->ops->tx_writefifo(pdata, (unsigned int *)bufp, wrsz);
 	freespace -= (skb->len + 32);
+	skb_tx_timestamp(skb);
 	dev_kfree_skb(skb);
 
 	if (unlikely(smsc911x_tx_get_txstatcount(pdata) >= 30))

-- 
1.7.0.4

This patch enables software (and phy device) transmit time stamping
Compile tested only.

Cc: Sachin Sanap <redacted>
Cc: Zhangfei Gao <redacted>
Cc: Philip Rakity <redacted>
Cc: Mark Brown <redacted>
Signed-off-by: Richard Cochran <redacted>
---
 drivers/net/pxa168_eth.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/pxa168_eth.c b/drivers/net/pxa168_eth.c
index 89f7540..cd2e471 100644
--- a/drivers/net/pxa168_eth.c
+++ b/drivers/net/pxa168_eth.c

@@ -1273,6 +1273,7 @@ static int pxa168_eth_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	wmb();
 	wrl(pep, SDMA_CMD, SDMA_CMD_TXDH | SDMA_CMD_ERD);
 
+	skb_tx_timestamp(skb);
 	stats->tx_bytes += skb->len;
 	stats->tx_packets++;
 	dev->trans_start = jiffies;

-- 
1.7.0.4

On Sun, Jun 19, 2011 at 01:20:05PM +0200, Richard Cochran wrote:

And the line above is unsafe, too.

 	stats->tx_packets++;
 	dev->trans_start = jiffies;
-- 
1.7.0.4

This patch enables software (and phy device) transmit time stamping.
Compile tested only.

Cc: Lennert Buytenhek <redacted>
Signed-off-by: Richard Cochran <redacted>
---
 drivers/net/mv643xx_eth.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
index a5d9b1c..c7a8f10 100644
--- a/drivers/net/mv643xx_eth.c
+++ b/drivers/net/mv643xx_eth.c

@@ -884,6 +884,8 @@ static netdev_tx_t mv643xx_eth_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (!txq_submit_skb(txq, skb)) {
 		int entries_left;
 
+		skb_tx_timestamp(skb);
+
 		txq->tx_bytes += skb->len;
 		txq->tx_packets++;
 

-- 
1.7.0.4

On Sun, Jun 19, 2011 at 01:20:06PM +0200, Richard Cochran wrote:

And the line above is unsafe, as well.

 		txq->tx_packets++;
 
-- 
1.7.0.4

Le dimanche 19 juin 2011 à 20:17 +0200, Richard Cochran a écrit :

On Sun, Jun 19, 2011 at 01:20:06PM +0200, Richard Cochran wrote:

quoted

This patch enables software (and phy device) transmit time stamping.
Compile tested only.

Cc: Lennert Buytenhek <redacted>
Signed-off-by: Richard Cochran <redacted>
---
 drivers/net/mv643xx_eth.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
index a5d9b1c..c7a8f10 100644
--- a/drivers/net/mv643xx_eth.c
+++ b/drivers/net/mv643xx_eth.c

@@ -884,6 +884,8 @@ static netdev_tx_t mv643xx_eth_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (!txq_submit_skb(txq, skb)) {
 		int entries_left;
 
+		skb_tx_timestamp(skb);
+
 		txq->tx_bytes += skb->len;

And the line above is unsafe, as well.

Yes, for sure, please submit patches to fix this (before adding time
stamping patches), as this should go to stable.

From: Eric Dumazet <redacted>
Date: Sun, 19 Jun 2011 20:33:18 +0200

Yes, for sure, please submit patches to fix this (before adding time
stamping patches), as this should go to stable.

Agreed.

As soon as skb is given to hardware and spinlock released, TX completion
can free skb under us. Therefore, we should update netdev stats before
spinlock release.

Signed-off-by: Eric Dumazet <redacted>
---
 drivers/net/hp100.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/hp100.c b/drivers/net/hp100.c
index 8e10d2f..c3ecb11 100644
--- a/drivers/net/hp100.c
+++ b/drivers/net/hp100.c

@@ -1580,12 +1580,12 @@ static netdev_tx_t hp100_start_xmit_bm(struct sk_buff *skb,
 	hp100_outl(ringptr->pdl_paddr, TX_PDA_L);	/* Low Prio. Queue */
 
 	lp->txrcommit++;
-	spin_unlock_irqrestore(&lp->lock, flags);
 
-	/* Update statistics */
 	dev->stats.tx_packets++;
 	dev->stats.tx_bytes += skb->len;
 
+	spin_unlock_irqrestore(&lp->lock, flags);
+
 	return NETDEV_TX_OK;
 
 drop:

From: Eric Dumazet <redacted>
Date: Mon, 20 Jun 2011 00:43:33 +0200

As soon as skb is given to hardware and spinlock released, TX completion
can free skb under us. Therefore, we should update netdev stats before
spinlock release.

Signed-off-by: Eric Dumazet <redacted>

Applied.

This patch enables software (and phy device) transmit time stamping.
Compile tested only.

Cc: Shlomi Gridish <redacted>
Cc: Li Yang <redacted>
Signed-off-by: Richard Cochran <redacted>
---
 drivers/net/ucc_geth.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
index ef04105..b091108 100644
--- a/drivers/net/ucc_geth.c
+++ b/drivers/net/ucc_geth.c

@@ -3179,6 +3179,7 @@ static int ucc_geth_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	out_be16(uccf->p_utodr, UCC_FAST_TOD);
 #endif
 	spin_unlock_irqrestore(&ugeth->lock, flags);
+	skb_tx_timestamp(skb);
 
 	return NETDEV_TX_OK;
 }

-- 
1.7.0.4