Date:	Thu, 17 Jul 2014 15:17:59 +0200
From:	Stefan Sørensen 
	<stefan.sorensen@...ctralink.com>
To:	<davem@...emloft.net>, <richardcochran@...il.com>
CC:	<netdev@...r.kernel.org>,
	Stefan Sørensen 
	<stefan.sorensen@...ctralink.com>
Subject: [PATCH net-next] dp83640: Fix receive timestamp race condition

When timestamping received packets, rx_timestamp_work may be scheduled before
the timestamps is received from the hardware resulting in the packet beeing
delivered without the timestamp.

This is fixed by changing the receive timestamp path: On receiving a packet
that need timestamping, it is added to a queue. When a timestamp arrives, the
queue is traversed and if a matching packet is found, it is deliverd with the
timestamp. In case the hardware drops a timestamp, a workqueue regularly
checks the queue for old packets and delivers them without a timestamp.

Signed-off-by: Stefan Sørensen <stefan.sorensen@...ctralink.com>
---
 drivers/net/phy/dp83640.c | 197 +++++++++++++++++++---------------------------
 1 file changed, 82 insertions(+), 115 deletions(-)

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 255c21f..89eea31 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -74,7 +74,10 @@
 #define ENDIAN_FLAG	PSF_ENDIAN
 #endif
 
-#define SKB_PTP_TYPE(__skb) (*(unsigned int *)((__skb)->cb))
+struct dp83640_skb_info {
+	int ptp_type;
+	unsigned long tmo;
+};
 
 struct phy_rxts {
 	u16 ns_lo;   /* ns[15:0] */
@@ -94,7 +97,6 @@ struct phy_txts {
 
 struct rxts {
 	struct list_head list;
-	unsigned long tmo;
 	u64 ns;
 	u16 seqid;
 	u8  msgtype;
@@ -116,12 +118,6 @@ struct dp83640_private {
 	int cfg0;
 	/* remember the last event time stamp */
 	struct phy_txts edata;
-	/* list of rx timestamps */
-	struct list_head rxts;
-	struct list_head rxpool;
-	struct rxts rx_pool_data[MAX_RXTS];
-	/* protects above three fields from concurrent access */
-	spinlock_t rx_lock;
 	/* queues of incoming and outgoing packets */
 	struct sk_buff_head rx_queue;
 	struct sk_buff_head tx_queue;
@@ -281,7 +277,6 @@ static void phy2rxts(struct phy_rxts *p, struct rxts *rxts)
 	rxts->seqid = p->seqid;
 	rxts->msgtype = (p->msgtype >> 12) & 0xf;
 	rxts->hash = p->msgtype & 0x0fff;
-	rxts->tmo = jiffies + 2;
 }
 
 static u64 phy2txts(struct phy_txts *p)
@@ -563,26 +558,6 @@ static bool is_status_frame(struct sk_buff *skb, int type)
 		return false;
 }
 
-static int expired(struct rxts *rxts)
-{
-	return time_after(jiffies, rxts->tmo);
-}
-
-/* Caller must hold rx_lock. */
-static void prune_rx_ts(struct dp83640_private *dp83640)
-{
-	struct list_head *this, *next;
-	struct rxts *rxts;
-
-	list_for_each_safe(this, next, &dp83640->rxts) {
-		rxts = list_entry(this, struct rxts, list);
-		if (expired(rxts)) {
-			list_del_init(&rxts->list);
-			list_add(&rxts->list, &dp83640->rxpool);
-		}
-	}
-}
-
 /* synchronize the phyters so they act as one clock */
 
 static void enable_broadcast(struct phy_device *phydev, int init_page, int on)
@@ -768,26 +743,70 @@ static int decode_evnt(struct dp83640_private *dp83640,
 	return parsed * sizeof(u16);
 }
 
+static int match(struct sk_buff *skb, unsigned int type, struct rxts *rxts)
+{
+	u16 *seqid;
+	unsigned int offset = 0;
+	u8 *msgtype, *data = skb_mac_header(skb);
+
+	/* check sequenceID, messageType, 12 bit hash of offset 20-29 */
+
+	if (type & PTP_CLASS_VLAN)
+		offset += VLAN_HLEN;
+
+	switch (type & PTP_CLASS_PMASK) {
+	case PTP_CLASS_IPV4:
+		offset += ETH_HLEN + IPV4_HLEN(data) + UDP_HLEN;
+		break;
+	case PTP_CLASS_IPV6:
+		offset += ETH_HLEN + IP6_HLEN + UDP_HLEN;
+		break;
+	case PTP_CLASS_L2:
+		offset += ETH_HLEN;
+		break;
+	default:
+		return 0;
+	}
+
+	if (skb->len + ETH_HLEN < offset + OFF_PTP_SEQUENCE_ID + sizeof(*seqid))
+		return 0;
+
+	if (unlikely(type & PTP_CLASS_V1))
+		msgtype = data + offset + OFF_PTP_CONTROL;
+	else
+		msgtype = data + offset;
+
+	seqid = (u16 *)(data + offset + OFF_PTP_SEQUENCE_ID);
+
+	return rxts->msgtype == (*msgtype & 0xf) &&
+		rxts->seqid   == ntohs(*seqid);
+}
+
 static void decode_rxts(struct dp83640_private *dp83640,
 			struct phy_rxts *phy_rxts)
 {
-	struct rxts *rxts;
+	struct rxts rxts;
+	struct skb_shared_hwtstamps *shhwtstamps;
+	struct sk_buff *skb;
 	unsigned long flags;
 
-	spin_lock_irqsave(&dp83640->rx_lock, flags);
+	phy2rxts(phy_rxts, &rxts);
 
-	prune_rx_ts(dp83640);
+	spin_lock_irqsave(&dp83640->rx_queue.lock, flags);
+	skb_queue_walk(&dp83640->rx_queue, skb) {
+		struct dp83640_skb_info *skb_info;
 
-	if (list_empty(&dp83640->rxpool)) {
-		pr_debug("rx timestamp pool is empty\n");
-		goto out;
+		skb_info = (struct dp83640_skb_info *)skb->cb;
+		if (match(skb, skb_info->ptp_type, &rxts)) {
+			__skb_unlink(skb, &dp83640->rx_queue);
+			shhwtstamps = skb_hwtstamps(skb);
+			memset(shhwtstamps, 0, sizeof(*shhwtstamps));
+			shhwtstamps->hwtstamp = ns_to_ktime(rxts.ns);
+			netif_rx_ni(skb);
+			break;
+		}
 	}
-	rxts = list_first_entry(&dp83640->rxpool, struct rxts, list);
-	list_del_init(&rxts->list);
-	phy2rxts(phy_rxts, rxts);
-	list_add_tail(&rxts->list, &dp83640->rxts);
-out:
-	spin_unlock_irqrestore(&dp83640->rx_lock, flags);
+	spin_unlock_irqrestore(&dp83640->rx_queue.lock, flags);
 }
 
 static void decode_txts(struct dp83640_private *dp83640,
@@ -887,45 +906,6 @@ static int is_sync(struct sk_buff *skb, int type)
 	return (*msgtype & 0xf) == 0;
 }
 
-static int match(struct sk_buff *skb, unsigned int type, struct rxts *rxts)
-{
-	u16 *seqid;
-	unsigned int offset = 0;
-	u8 *msgtype, *data = skb_mac_header(skb);
-
-	/* check sequenceID, messageType, 12 bit hash of offset 20-29 */
-
-	if (type & PTP_CLASS_VLAN)
-		offset += VLAN_HLEN;
-
-	switch (type & PTP_CLASS_PMASK) {
-	case PTP_CLASS_IPV4:
-		offset += ETH_HLEN + IPV4_HLEN(data) + UDP_HLEN;
-		break;
-	case PTP_CLASS_IPV6:
-		offset += ETH_HLEN + IP6_HLEN + UDP_HLEN;
-		break;
-	case PTP_CLASS_L2:
-		offset += ETH_HLEN;
-		break;
-	default:
-		return 0;
-	}
-
-	if (skb->len + ETH_HLEN < offset + OFF_PTP_SEQUENCE_ID + sizeof(*seqid))
-		return 0;
-
-	if (unlikely(type & PTP_CLASS_V1))
-		msgtype = data + offset + OFF_PTP_CONTROL;
-	else
-		msgtype = data + offset;
-
-	seqid = (u16 *)(data + offset + OFF_PTP_SEQUENCE_ID);
-
-	return rxts->msgtype == (*msgtype & 0xf) &&
-		rxts->seqid   == ntohs(*seqid);
-}
-
 static void dp83640_free_clocks(void)
 {
 	struct dp83640_clock *clock;
@@ -1049,7 +1029,7 @@ static int dp83640_probe(struct phy_device *phydev)
 {
 	struct dp83640_clock *clock;
 	struct dp83640_private *dp83640;
-	int err = -ENOMEM, i;
+	int err = -ENOMEM;
 
 	if (phydev->addr == BROADCAST_ADDR)
 		return 0;
@@ -1065,14 +1045,8 @@ static int dp83640_probe(struct phy_device *phydev)
 	dp83640->phydev = phydev;
 	INIT_WORK(&dp83640->ts_work, rx_timestamp_work);
 
-	INIT_LIST_HEAD(&dp83640->rxts);
-	INIT_LIST_HEAD(&dp83640->rxpool);
-	for (i = 0; i < MAX_RXTS; i++)
-		list_add(&dp83640->rx_pool_data[i].list, &dp83640->rxpool);
-
 	phydev->priv = dp83640;
 
-	spin_lock_init(&dp83640->rx_lock);
 	skb_queue_head_init(&dp83640->rx_queue);
 	skb_queue_head_init(&dp83640->tx_queue);
 
@@ -1267,6 +1241,13 @@ static int dp83640_hwtstamp(struct phy_device *phydev, struct ifreq *ifr)
 		return -ERANGE;
 	}
 
+	if (!dp83640->hwts_rx_en) {
+		struct sk_buff *skb;
+
+		while ((skb = skb_dequeue(&dp83640->rx_queue)))
+			netif_rx_ni(skb);
+	}
+
 	txcfg0 = (dp83640->version & TX_PTP_VER_MASK) << TX_PTP_VER_SHIFT;
 	rxcfg0 = (dp83640->version & TX_PTP_VER_MASK) << TX_PTP_VER_SHIFT;
 
@@ -1302,44 +1283,30 @@ static void rx_timestamp_work(struct work_struct *work)
 {
 	struct dp83640_private *dp83640 =
 		container_of(work, struct dp83640_private, ts_work);
-	struct list_head *this, *next;
-	struct rxts *rxts;
-	struct skb_shared_hwtstamps *shhwtstamps;
 	struct sk_buff *skb;
-	unsigned int type;
-	unsigned long flags;
 
-	/* Deliver each deferred packet, with or without a time stamp. */
-
-	while ((skb = skb_dequeue(&dp83640->rx_queue)) != NULL) {
-		type = SKB_PTP_TYPE(skb);
-		spin_lock_irqsave(&dp83640->rx_lock, flags);
-		list_for_each_safe(this, next, &dp83640->rxts) {
-			rxts = list_entry(this, struct rxts, list);
-			if (match(skb, type, rxts)) {
-				shhwtstamps = skb_hwtstamps(skb);
-				memset(shhwtstamps, 0, sizeof(*shhwtstamps));
-				shhwtstamps->hwtstamp = ns_to_ktime(rxts->ns);
-				list_del_init(&rxts->list);
-				list_add(&rxts->list, &dp83640->rxpool);
-				break;
-			}
+	/* Deliver expired packets. */
+	while ((skb = skb_dequeue(&dp83640->rx_queue))) {
+		struct dp83640_skb_info *skb_info;
+
+		skb_info = (struct dp83640_skb_info *)skb->cb;
+		if (!time_after(jiffies, skb_info->tmo)) {
+			skb_queue_head(&dp83640->rx_queue, skb);
+			break;
 		}
-		spin_unlock_irqrestore(&dp83640->rx_lock, flags);
+
 		netif_rx_ni(skb);
 	}
 
-	/* Clear out expired time stamps. */
-
-	spin_lock_irqsave(&dp83640->rx_lock, flags);
-	prune_rx_ts(dp83640);
-	spin_unlock_irqrestore(&dp83640->rx_lock, flags);
+	if (!skb_queue_empty(&dp83640->rx_queue))
+		schedule_work(&dp83640->ts_work);
 }
 
 static bool dp83640_rxtstamp(struct phy_device *phydev,
 			     struct sk_buff *skb, int type)
 {
 	struct dp83640_private *dp83640 = phydev->priv;
+	struct dp83640_skb_info *skb_info = (struct dp83640_skb_info *)skb->cb;
 
 	if (is_status_frame(skb, type)) {
 		decode_status_frame(dp83640, skb);
@@ -1350,7 +1317,8 @@ static bool dp83640_rxtstamp(struct phy_device *phydev,
 	if (!dp83640->hwts_rx_en)
 		return false;
 
-	SKB_PTP_TYPE(skb) = type;
+	skb_info->ptp_type = type;
+	skb_info->tmo = jiffies + 2;
 	skb_queue_tail(&dp83640->rx_queue, skb);
 	schedule_work(&dp83640->ts_work);
 
@@ -1373,7 +1341,6 @@ static void dp83640_txtstamp(struct phy_device *phydev,
 	case HWTSTAMP_TX_ON:
 		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 		skb_queue_tail(&dp83640->tx_queue, skb);
-		schedule_work(&dp83640->ts_work);
 		break;
 
 	case HWTSTAMP_TX_OFF:
-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists