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

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

[Richard Cochran]

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

[PATCH 01/11] net: export the receive time stamping hook for non-NAPI drivers

[Richard Cochran]

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 <richard.cochran@omicron.at>
---
 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

[PATCH 02/11] lib8390: enable transmit and receive time stamping.

[Richard Cochran]

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 <richard.cochran@omicron.at>
---
 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

[PATCH 03/11] emaclite: enable transmit and receive time stamping.

[Richard Cochran]

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 <john.linn@xilinx.com>
Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 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

[PATCH 04/11] ll_temac: enable transmit and receive time stamping.

[Richard Cochran]

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 <grant.likely@secretlab.ca>
Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 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

[PATCH 05/11] fec_mpc52xx: enable transmit and receive time stamping.

[Richard Cochran]

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 <grant.likely@secretlab.ca>
Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 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

[PATCH 06/11] macb: enable transmit time stamping.

[Richard Cochran]

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

Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 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

[PATCH 07/11] fs_enet: enable transmit time stamping.

[Richard Cochran]

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

Cc: Pantelis Antoniou <pantelis.antoniou@gmail.com>
Cc: Vitaly Bordug <vbordug@ru.mvista.com>
Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 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

[PATCH 08/11] smsc911x: enable transmit time stamping.

[Richard Cochran]

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

Cc: Steve Glendinning <steve.glendinning@smsc.com>
Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 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

[PATCH 09/11] pxa168_eth: enable transmit time stamping.

[Richard Cochran]

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

Cc: Sachin Sanap <ssanap@marvell.com>
Cc: Zhangfei Gao <zgao6@marvell.com>
Cc: Philip Rakity <prakity@marvell.com>
Cc: Mark Brown <markb@marvell.com>
Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 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

[PATCH 10/11] mv643xx_eth: enable transmit time stamping.

[Richard Cochran]

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

Cc: Lennert Buytenhek <buytenh@wantstofly.org>
Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 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

[PATCH 11/11] ucc_geth: enable transmit time stamping.

[Richard Cochran]

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

Cc: Shlomi Gridish <gridish@freescale.com>
Cc: Li Yang <leoli@freescale.com>
Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 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

Re: [PATCH 03/11] emaclite: enable transmit and receive time stamping.

[Eric Dumazet]

Le dimanche 19 juin 2011 à 13:19 +0200, Richard Cochran a écrit :
> 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 <john.linn@xilinx.com>
> Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
> ---
>  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);

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

Re: [PATCH 05/11] fec_mpc52xx: enable transmit and receive time stamping.

[Eric Dumazet]

Le dimanche 19 juin 2011 à 13:20 +0200, Richard Cochran a écrit :
> 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 <grant.likely@secretlab.ca>
> Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
> ---
>  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);

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

> +	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);
>  	}

Re: [PATCH 06/11] macb: enable transmit time stamping.

[Eric Dumazet]

Le dimanche 19 juin 2011 à 13:20 +0200, Richard Cochran a écrit :
> This patch enables software (and phy device) transmit time stamping
> Compile tested only.
> 
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
> ---
>  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;
>  }
>  

Same problem here, its not safe.

Re: [PATCH 07/11] fs_enet: enable transmit time stamping.

[Eric Dumazet]

Le dimanche 19 juin 2011 à 13:20 +0200, Richard Cochran a écrit :
> This patch enables software (and phy device) transmit time stamping.
> Compile tested only.
> 
> Cc: Pantelis Antoniou <pantelis.antoniou@gmail.com>
> Cc: Vitaly Bordug <vbordug@ru.mvista.com>
> Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
> ---
>  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;
>  }
>  

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

Re: [PATCH 07/11] fs_enet: enable transmit time stamping.

[Richard Cochran]

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

Re: [PATCH 09/11] pxa168_eth: enable transmit time stamping.

[Richard Cochran]

On Sun, Jun 19, 2011 at 01:20:05PM +0200, Richard Cochran wrote:
> This patch enables software (and phy device) transmit time stamping
> Compile tested only.
> 
> Cc: Sachin Sanap <ssanap@marvell.com>
> Cc: Zhangfei Gao <zgao6@marvell.com>
> Cc: Philip Rakity <prakity@marvell.com>
> Cc: Mark Brown <markb@marvell.com>
> Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
> ---
>  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;

And the line above is unsafe, too.

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

Re: [PATCH 10/11] mv643xx_eth: enable transmit time stamping.

[Richard Cochran]

On Sun, Jun 19, 2011 at 01:20:06PM +0200, Richard Cochran wrote:
> This patch enables software (and phy device) transmit time stamping.
> Compile tested only.
> 
> Cc: Lennert Buytenhek <buytenh@wantstofly.org>
> Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
> ---
>  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.

>  		txq->tx_packets++;
>  
> -- 
> 1.7.0.4
> 

Re: [PATCH 07/11] fs_enet: enable transmit time stamping.

[Eric Dumazet]

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

Re: [PATCH 10/11] mv643xx_eth: enable transmit time stamping.

[Eric Dumazet]

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:
> > This patch enables software (and phy device) transmit time stamping.
> > Compile tested only.
> > 
> > Cc: Lennert Buytenhek <buytenh@wantstofly.org>
> > Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
> > ---
> >  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.

Re: [PATCH 10/11] mv643xx_eth: enable transmit time stamping.

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
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.

[PATCH] hp100: fix an skb->len race

[Eric Dumazet]

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 <eric.dumazet@gmail.com>
---
 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:

Re: [PATCH] hp100: fix an skb->len race

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
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 <eric.dumazet@gmail.com>

Applied.

Re: [PATCH 07/11] fs_enet: enable transmit time stamping.

[Richard Cochran]

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

Re: [PATCH 07/11] fs_enet: enable transmit time stamping.

[Matt Carlson]

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:
> > 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.
> 
> 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?