[PATCH 01/11] netdev: bfin_mac: add support for IEEE 1588 PTP

[PATCH 01/11] netdev: bfin_mac: add support for IEEE 1588 PTP

[Mike Frysinger]

From: Barry Song <barry.song-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>

Newer on-chip MAC peripherals support IEEE 1588 PTP in the hardware, so
extend the driver to support this functionality.

Signed-off-by: Barry Song <barry.song-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
---
 drivers/net/Kconfig    |    7 +
 drivers/net/bfin_mac.c |  341 +++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/net/bfin_mac.h |   15 ++
 3 files changed, 362 insertions(+), 1 deletions(-)

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 7b832c7..ac536b9 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -887,6 +887,13 @@ config BFIN_MAC_RMII
 	help
 	  Use Reduced PHY MII Interface
 
+config BFIN_MAC_USE_HWSTAMP
+	bool "Use IEEE 1588 hwstamp"
+	depends on BFIN_MAC && BF518
+	default y
+	help
+	  To support the IEEE 1588 Precision Time Protocol (PTP), select y here
+
 config SMC9194
 	tristate "SMC 9194 support"
 	depends on NET_VENDOR_SMC && (ISA || MAC && BROKEN)
diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
index 587f93c..6a9519f 100644
--- a/drivers/net/bfin_mac.c
+++ b/drivers/net/bfin_mac.c
@@ -33,6 +33,7 @@
 #include <asm/dma.h>
 #include <linux/dma-mapping.h>
 
+#include <asm/div64.h>
 #include <asm/dpmc.h>
 #include <asm/blackfin.h>
 #include <asm/cacheflush.h>
@@ -551,6 +552,309 @@ static int bfin_mac_set_mac_address(struct net_device *dev, void *p)
 	return 0;
 }
 
+#ifdef CONFIG_BFIN_MAC_USE_HWSTAMP
+#define bfin_mac_hwtstamp_is_none(cfg) ((cfg) == HWTSTAMP_FILTER_NONE)
+
+static int bfin_mac_hwtstamp_ioctl(struct net_device *netdev,
+		struct ifreq *ifr, int cmd)
+{
+	struct hwtstamp_config config;
+	struct bfin_mac_local *lp = netdev_priv(netdev);
+	u16 ptpctl;
+	u32 ptpfv1, ptpfv2, ptpfv3, ptpfoff;
+
+	if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
+		return -EFAULT;
+
+	pr_debug("%s config flag:0x%x, tx_type:0x%x, rx_filter:0x%x\n",
+			__func__, config.flags, config.tx_type, config.rx_filter);
+
+	/* reserved for future extensions */
+	if (config.flags)
+		return -EINVAL;
+
+	if ((config.tx_type != HWTSTAMP_TX_OFF) &&
+			(config.tx_type != HWTSTAMP_TX_ON))
+		return -ERANGE;
+
+	ptpctl = bfin_read_EMAC_PTP_CTL();
+
+	switch (config.rx_filter) {
+	case HWTSTAMP_FILTER_NONE:
+		/*
+		 * Dont allow any timestamping
+		 */
+		ptpfv3 = 0xFFFFFFFF;
+		bfin_write_EMAC_PTP_FV3(ptpfv3);
+		break;
+	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
+		/*
+		 * Clear the five comparison mask bits (bits[12:8]) in EMAC_PTP_CTL)
+		 * to enable all the field matches.
+		 */
+		ptpctl &= ~0x1F00;
+		bfin_write_EMAC_PTP_CTL(ptpctl);
+		/*
+		 * Keep the default values of the EMAC_PTP_FOFF register.
+		 */
+		ptpfoff = 0x4A24170C;
+		bfin_write_EMAC_PTP_FOFF(ptpfoff);
+		/*
+		 * Keep the default values of the EMAC_PTP_FV1 and EMAC_PTP_FV2
+		 * registers.
+		 */
+		ptpfv1 = 0x11040800;
+		bfin_write_EMAC_PTP_FV1(ptpfv1);
+		ptpfv2 = 0x0140013F;
+		bfin_write_EMAC_PTP_FV2(ptpfv2);
+		/*
+		 * The default value (0xFFFC) allows the timestamping of both
+		 * received Sync messages and Delay_Req messages.
+		 */
+		ptpfv3 = 0xFFFFFFFC;
+		bfin_write_EMAC_PTP_FV3(ptpfv3);
+
+		config.rx_filter = HWTSTAMP_FILTER_PTP_V1_L4_EVENT;
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+		/* Clear all five comparison mask bits (bits[12:8]) in the
+		 * EMAC_PTP_CTL register to enable all the field matches.
+		 */
+		ptpctl &= ~0x1F00;
+		bfin_write_EMAC_PTP_CTL(ptpctl);
+		/*
+		 * Keep the default values of the EMAC_PTP_FOFF register, except set
+		 * the PTPCOF field to 0x2A.
+		 */
+		ptpfoff = 0x2A24170C;
+		bfin_write_EMAC_PTP_FOFF(ptpfoff);
+		/*
+		 * Keep the default values of the EMAC_PTP_FV1 and EMAC_PTP_FV2
+		 * registers.
+		 */
+		ptpfv1 = 0x11040800;
+		bfin_write_EMAC_PTP_FV1(ptpfv1);
+		ptpfv2 = 0x0140013F;
+		bfin_write_EMAC_PTP_FV2(ptpfv2);
+		/*
+		 * To allow the timestamping of Pdelay_Req and Pdelay_Resp, set
+		 * the value to 0xFFF0.
+		 */
+		ptpfv3 = 0xFFFFFFF0;
+		bfin_write_EMAC_PTP_FV3(ptpfv3);
+
+		config.rx_filter = HWTSTAMP_FILTER_PTP_V2_L4_EVENT;
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
+		/*
+		 * Clear bits 8 and 12 of the EMAC_PTP_CTL register to enable only the
+		 * EFTM and PTPCM field comparison.
+		 */
+		ptpctl &= ~0x1100;
+		bfin_write_EMAC_PTP_CTL(ptpctl);
+		/*
+		 * Keep the default values of all the fields of the EMAC_PTP_FOFF
+		 * register, except set the PTPCOF field to 0x0E.
+		 */
+		ptpfoff = 0x0E24170C;
+		bfin_write_EMAC_PTP_FOFF(ptpfoff);
+		/*
+		 * Program bits [15:0] of the EMAC_PTP_FV1 register to 0x88F7, which
+		 * corresponds to PTP messages on the MAC layer.
+		 */
+		ptpfv1 = 0x110488F7;
+		bfin_write_EMAC_PTP_FV1(ptpfv1);
+		ptpfv2 = 0x0140013F;
+		bfin_write_EMAC_PTP_FV2(ptpfv2);
+		/*
+		 * To allow the timestamping of Pdelay_Req and Pdelay_Resp
+		 * messages, set the value to 0xFFF0.
+		 */
+		ptpfv3 = 0xFFFFFFF0;
+		bfin_write_EMAC_PTP_FV3(ptpfv3);
+
+		config.rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	if (config.tx_type == HWTSTAMP_TX_OFF &&
+	    bfin_mac_hwtstamp_is_none(config.rx_filter)) {
+		ptpctl &= ~PTP_EN;
+		bfin_write_EMAC_PTP_CTL(ptpctl);
+
+		SSYNC();
+	} else {
+		ptpctl |= PTP_EN;
+		bfin_write_EMAC_PTP_CTL(ptpctl);
+
+		/*
+		 * clear any existing timestamp
+		 */
+		bfin_read_EMAC_PTP_RXSNAPLO();
+		bfin_read_EMAC_PTP_RXSNAPHI();
+
+		bfin_read_EMAC_PTP_TXSNAPLO();
+		bfin_read_EMAC_PTP_TXSNAPHI();
+
+		/*
+		 * Set registers so that rollover occurs soon to test this.
+		 */
+		bfin_write_EMAC_PTP_TIMELO(0x00000000);
+		bfin_write_EMAC_PTP_TIMEHI(0xFF800000);
+
+		SSYNC();
+
+		lp->compare.last_update = 0;
+		timecounter_init(&lp->clock,
+				&lp->cycles,
+				ktime_to_ns(ktime_get_real()));
+		timecompare_update(&lp->compare, 0);
+	}
+
+	lp->stamp_cfg = config;
+	return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ?
+		-EFAULT : 0;
+}
+
+static void bfin_dump_hwtamp(char *s, ktime_t *hw, ktime_t *ts, struct timecompare *cmp)
+{
+	ktime_t sys = ktime_get_real();
+
+	pr_debug("%s %s hardware:%d,%d transform system:%d,%d system:%d,%d, cmp:%lld, %lld\n",
+			__func__, s, hw->tv.sec, hw->tv.nsec, ts->tv.sec, ts->tv.nsec, sys.tv.sec,
+			sys.tv.nsec, cmp->offset, cmp->skew);
+}
+
+static void bfin_tx_hwtstamp(struct net_device *netdev, struct sk_buff *skb)
+{
+	struct bfin_mac_local *lp = netdev_priv(netdev);
+	union skb_shared_tx *shtx = skb_tx(skb);
+
+	if (shtx->hardware) {
+		int timeout_cnt = MAX_TIMEOUT_CNT;
+
+		/* When doing time stamping, keep the connection to the socket
+		 * a while longer
+		 */
+		shtx->in_progress = 1;
+
+		/*
+		 * The timestamping is done at the EMAC module's MII/RMII interface
+		 * when the module sees the Start of Frame of an event message packet. This
+		 * interface is the closest possible place to the physical Ethernet transmission
+		 * medium, providing the best timing accuracy.
+		 */
+		while ((!(bfin_read_EMAC_PTP_ISTAT() & TXTL)) && (--timeout_cnt))
+			udelay(1);
+		if (timeout_cnt == 0)
+			printk(KERN_ERR DRV_NAME
+					": fails to timestamp the TX packet\n");
+		else {
+			struct skb_shared_hwtstamps shhwtstamps;
+			u64 ns;
+			u64 regval;
+
+			regval = bfin_read_EMAC_PTP_TXSNAPLO();
+			regval |= (u64)bfin_read_EMAC_PTP_TXSNAPHI() << 32;
+			memset(&shhwtstamps, 0, sizeof(shhwtstamps));
+			ns = timecounter_cyc2time(&lp->clock,
+					regval);
+			timecompare_update(&lp->compare, ns);
+			shhwtstamps.hwtstamp = ns_to_ktime(ns);
+			shhwtstamps.syststamp =
+				timecompare_transform(&lp->compare, ns);
+			skb_tstamp_tx(skb, &shhwtstamps);
+
+			bfin_dump_hwtamp("TX", &shhwtstamps.hwtstamp, &shhwtstamps.syststamp, &lp->compare);
+		}
+	}
+}
+
+static void bfin_rx_hwtstamp(struct net_device *netdev, struct sk_buff *skb)
+{
+	struct bfin_mac_local *lp = netdev_priv(netdev);
+	u32 valid;
+	u64 regval, ns;
+	struct skb_shared_hwtstamps *shhwtstamps;
+
+	if (bfin_mac_hwtstamp_is_none(lp->stamp_cfg.rx_filter))
+		return;
+
+	valid = bfin_read_EMAC_PTP_ISTAT() & RXEL;
+	if (!valid)
+		return;
+
+	shhwtstamps = skb_hwtstamps(skb);
+
+	regval = bfin_read_EMAC_PTP_RXSNAPLO();
+	regval |= (u64)bfin_read_EMAC_PTP_RXSNAPHI() << 32;
+	ns = timecounter_cyc2time(&lp->clock, regval);
+	timecompare_update(&lp->compare, ns);
+	memset(shhwtstamps, 0, sizeof(*shhwtstamps));
+	shhwtstamps->hwtstamp = ns_to_ktime(ns);
+	shhwtstamps->syststamp = timecompare_transform(&lp->compare, ns);
+
+	bfin_dump_hwtamp("RX", &shhwtstamps->hwtstamp, &shhwtstamps->syststamp, &lp->compare);
+}
+
+/*
+ * bfin_read_clock - read raw cycle counter (to be used by time counter)
+ */
+static cycle_t bfin_read_clock(const struct cyclecounter *tc)
+{
+	u64 stamp;
+
+	stamp =  bfin_read_EMAC_PTP_TIMELO();
+	stamp |= (u64)bfin_read_EMAC_PTP_TIMEHI() << 32ULL;
+
+	return stamp;
+}
+
+#define PTP_CLK 25000000
+
+static void bfin_mac_hwtstamp_init(struct net_device *netdev)
+{
+	struct bfin_mac_local *lp = netdev_priv(netdev);
+	u64 append;
+
+	/* Initialize hardware timer */
+	append = PTP_CLK * (1ULL << 32);
+	do_div(append, get_sclk());
+	bfin_write_EMAC_PTP_ADDEND((u32)append);
+
+	memset(&lp->cycles, 0, sizeof(lp->cycles));
+	lp->cycles.read = bfin_read_clock;
+	lp->cycles.mask = CLOCKSOURCE_MASK(64);
+	lp->cycles.mult = 1000000000 / PTP_CLK;
+	lp->cycles.shift = 0;
+
+	/* Synchronize our NIC clock against system wall clock */
+	memset(&lp->compare, 0, sizeof(lp->compare));
+	lp->compare.source = &lp->clock;
+	lp->compare.target = ktime_get_real;
+	lp->compare.num_samples = 10;
+
+	/* Initialize hwstamp config */
+	lp->stamp_cfg.rx_filter = HWTSTAMP_FILTER_NONE;
+	lp->stamp_cfg.tx_type = HWTSTAMP_TX_OFF;
+}
+
+#else
+# define bfin_mac_hwtstamp_is_none(cfg) 0
+# define bfin_mac_hwtstamp_init(dev)
+# define bfin_mac_hwtstamp_ioctl(dev, ifr, cmd) (-EOPNOTSUPP)
+# define bfin_rx_hwtstamp(dev, skb)
+# define bfin_tx_hwtstamp(dev, skb)
+#endif
+
 static void adjust_tx_list(void)
 {
 	int timeout_cnt = MAX_TIMEOUT_CNT;
@@ -608,18 +912,32 @@ static int bfin_mac_hard_start_xmit(struct sk_buff *skb,
 {
 	u16 *data;
 	u32 data_align = (unsigned long)(skb->data) & 0x3;
+	union skb_shared_tx *shtx = skb_tx(skb);
+
 	current_tx_ptr->skb = skb;
 
 	if (data_align == 0x2) {
 		/* move skb->data to current_tx_ptr payload */
 		data = (u16 *)(skb->data) - 1;
-				*data = (u16)(skb->len);
+		*data = (u16)(skb->len);
+		/*
+		 * When transmitting an Ethernet packet, the PTP_TSYNC module requires
+		 * a DMA_Length_Word field associated with the packet. The lower 12 bits
+		 * of this field are the length of the packet payload in bytes and the higher
+		 * 4 bits are the timestamping enable field.
+		 */
+		if (shtx->hardware)
+			*data |= 0x1000;
+
 		current_tx_ptr->desc_a.start_addr = (u32)data;
 		/* this is important! */
 		blackfin_dcache_flush_range((u32)data,
 				(u32)((u8 *)data + skb->len + 4));
 	} else {
 		*((u16 *)(current_tx_ptr->packet)) = (u16)(skb->len);
+		/* enable timestamping for the sent packet */
+		if (shtx->hardware)
+			*((u16 *)(current_tx_ptr->packet)) |= 0x1000;
 		memcpy((u8 *)(current_tx_ptr->packet + 2), skb->data,
 			skb->len);
 		current_tx_ptr->desc_a.start_addr =
@@ -653,6 +971,9 @@ static int bfin_mac_hard_start_xmit(struct sk_buff *skb,
 
 out:
 	adjust_tx_list();
+
+	bfin_tx_hwtstamp(dev, skb);
+
 	current_tx_ptr = current_tx_ptr->next;
 	dev->trans_start = jiffies;
 	dev->stats.tx_packets++;
@@ -664,9 +985,11 @@ static void bfin_mac_rx(struct net_device *dev)
 {
 	struct sk_buff *skb, *new_skb;
 	unsigned short len;
+	struct bfin_mac_local *lp __maybe_unused = netdev_priv(dev);
 
 	/* allocate a new skb for next time receive */
 	skb = current_rx_ptr->skb;
+
 	new_skb = dev_alloc_skb(PKT_BUF_SZ + NET_IP_ALIGN);
 	if (!new_skb) {
 		printk(KERN_NOTICE DRV_NAME
@@ -691,6 +1014,9 @@ static void bfin_mac_rx(struct net_device *dev)
 					 (unsigned long)skb->tail);
 
 	skb->protocol = eth_type_trans(skb, dev);
+
+	bfin_rx_hwtstamp(dev, skb);
+
 #if defined(BFIN_MAC_CSUM_OFFLOAD)
 	skb->csum = current_rx_ptr->status.ip_payload_csum;
 	skb->ip_summed = CHECKSUM_COMPLETE;
@@ -874,6 +1200,16 @@ static void bfin_mac_set_multicast_list(struct net_device *dev)
 	}
 }
 
+static int bfin_mac_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
+{
+	switch (cmd) {
+	case SIOCSHWTSTAMP:
+		return bfin_mac_hwtstamp_ioctl(netdev, ifr, cmd);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 /*
  * this puts the device in an inactive state
  */
@@ -958,6 +1294,7 @@ static const struct net_device_ops bfin_mac_netdev_ops = {
 	.ndo_set_mac_address	= bfin_mac_set_mac_address,
 	.ndo_tx_timeout		= bfin_mac_timeout,
 	.ndo_set_multicast_list	= bfin_mac_set_multicast_list,
+	.ndo_do_ioctl           = bfin_mac_ioctl,
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_change_mtu		= eth_change_mtu,
 #ifdef CONFIG_NET_POLL_CONTROLLER
@@ -1049,6 +1386,8 @@ static int __devinit bfin_mac_probe(struct platform_device *pdev)
 		goto out_err_reg_ndev;
 	}
 
+	bfin_mac_hwtstamp_init(ndev);
+
 	/* now, print out the card info, in a short format.. */
 	dev_info(&pdev->dev, "%s, Version %s\n", DRV_DESC, DRV_VERSION);
 
diff --git a/drivers/net/bfin_mac.h b/drivers/net/bfin_mac.h
index 052b5dc..87c454f 100644
--- a/drivers/net/bfin_mac.h
+++ b/drivers/net/bfin_mac.h
@@ -7,6 +7,12 @@
  *
  * Licensed under the GPL-2 or later.
  */
+#ifndef _BFIN_MAC_H_
+#define _BFIN_MAC_H_
+
+#include <linux/net_tstamp.h>
+#include <linux/clocksource.h>
+#include <linux/timecompare.h>
 
 #define BFIN_MAC_CSUM_OFFLOAD
 
@@ -67,6 +73,15 @@ struct bfin_mac_local {
 
 	struct phy_device *phydev;
 	struct mii_bus *mii_bus;
+
+#if defined(CONFIG_BFIN_MAC_USE_HWSTAMP)
+	struct cyclecounter cycles;
+	struct timecounter clock;
+	struct timecompare compare;
+	struct hwtstamp_config stamp_cfg;
+#endif
 };
 
 extern void bfin_get_ether_addr(char *addr);
+
+#endif
-- 
1.7.1

[PATCH 02/11] netdev: bfin_mac: handler RX status errors

[Mike Frysinger]

From: Peter Meerwald <pmeerw@pmeerw.net>

Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
Signed-off-by: Graf Yang <graf.yang@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 drivers/net/bfin_mac.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
index 6a9519f..c888465 100644
--- a/drivers/net/bfin_mac.c
+++ b/drivers/net/bfin_mac.c
@@ -981,12 +981,25 @@ out:
 	return NETDEV_TX_OK;
 }
 
+#define RX_ERROR_MASK (RX_LONG | RX_ALIGN | RX_CRC | RX_LEN | \
+	RX_FRAG | RX_ADDR | RX_DMAO | RX_PHY | RX_LATE | RX_RANGE)
+
 static void bfin_mac_rx(struct net_device *dev)
 {
 	struct sk_buff *skb, *new_skb;
 	unsigned short len;
 	struct bfin_mac_local *lp __maybe_unused = netdev_priv(dev);
 
+	/* check if frame status word reports an error condition
+	 * we which case we simply drop the packet
+	 */
+	if (current_rx_ptr->status.status_word & RX_ERROR_MASK) {
+		printk(KERN_NOTICE DRV_NAME
+		       ": rx: receive error - packet dropped\n");
+		dev->stats.rx_dropped++;
+		goto out;
+	}
+
 	/* allocate a new skb for next time receive */
 	skb = current_rx_ptr->skb;
 
@@ -1025,11 +1038,9 @@ static void bfin_mac_rx(struct net_device *dev)
 	netif_rx(skb);
 	dev->stats.rx_packets++;
 	dev->stats.rx_bytes += len;
+out:
 	current_rx_ptr->status.status_word = 0x00000000;
 	current_rx_ptr = current_rx_ptr->next;
-
-out:
-	return;
 }
 
 /* interrupt routine to handle rx and error signal */
-- 
1.7.1

[PATCH 03/11] netdev: bfin_mac: invalid data cache only once for each new rx skb buffer

[Mike Frysinger]

From: Sonic Zhang <sonic.zhang@analog.com>

The skb buffer isn't actually used until we finish transferring and pass
it up to higher layers, so only invalidate the range once before we start
receiving actual data.  This also avoids the problem with data invalidating
on Blackfin systems -- there is no invalidate-only, just invalidate+flush.
So when running in writeback mode, there is the small (but not uncommon)
possibility of the flush overwriting valid DMA-ed data from the cache.

Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 drivers/net/bfin_mac.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
index c888465..f9ba598 100644
--- a/drivers/net/bfin_mac.c
+++ b/drivers/net/bfin_mac.c
@@ -203,6 +203,11 @@ static int desc_list_init(void)
 			goto init_error;
 		}
 		skb_reserve(new_skb, NET_IP_ALIGN);
+		/* Invidate the data cache of skb->data range when it is write back
+		 * cache. It will prevent overwritting the new data from DMA
+		 */
+		blackfin_dcache_invalidate_range((unsigned long)new_skb->head,
+					 (unsigned long)new_skb->end);
 		r->skb = new_skb;
 
 		/*
@@ -1012,19 +1017,17 @@ static void bfin_mac_rx(struct net_device *dev)
 	}
 	/* reserve 2 bytes for RXDWA padding */
 	skb_reserve(new_skb, NET_IP_ALIGN);
-	current_rx_ptr->skb = new_skb;
-	current_rx_ptr->desc_a.start_addr = (unsigned long)new_skb->data - 2;
-
 	/* Invidate the data cache of skb->data range when it is write back
 	 * cache. It will prevent overwritting the new data from DMA
 	 */
 	blackfin_dcache_invalidate_range((unsigned long)new_skb->head,
 					 (unsigned long)new_skb->end);
 
+	current_rx_ptr->skb = new_skb;
+	current_rx_ptr->desc_a.start_addr = (unsigned long)new_skb->data - 2;
+
 	len = (unsigned short)((current_rx_ptr->status.status_word) & RX_FRLEN);
 	skb_put(skb, len);
-	blackfin_dcache_invalidate_range((unsigned long)skb->head,
-					 (unsigned long)skb->tail);
 
 	skb->protocol = eth_type_trans(skb, dev);
 
-- 
1.7.1

[PATCH 04/11] netdev: bfin_mac: deduce Ethernet FCS from hardware IP payload checksum

[Mike Frysinger]

From: Sonic Zhang <sonic.zhang-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>

IP checksum is based on 16-bit one's complement algorithm.
To deduce a value from checksum is equal to add its complement.

Signed-off-by: Sonic Zhang <sonic.zhang-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
---
 drivers/net/bfin_mac.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
index f9ba598..5b00fc8 100644
--- a/drivers/net/bfin_mac.c
+++ b/drivers/net/bfin_mac.c
@@ -994,6 +994,10 @@ static void bfin_mac_rx(struct net_device *dev)
 	struct sk_buff *skb, *new_skb;
 	unsigned short len;
 	struct bfin_mac_local *lp __maybe_unused = netdev_priv(dev);
+#if defined(BFIN_MAC_CSUM_OFFLOAD)
+	unsigned int i;
+	unsigned char fcs[ETH_FCS_LEN + 1];
+#endif
 
 	/* check if frame status word reports an error condition
 	 * we which case we simply drop the packet
@@ -1027,6 +1031,8 @@ static void bfin_mac_rx(struct net_device *dev)
 	current_rx_ptr->desc_a.start_addr = (unsigned long)new_skb->data - 2;
 
 	len = (unsigned short)((current_rx_ptr->status.status_word) & RX_FRLEN);
+	/* Deduce Ethernet FCS length from Ethernet payload length */
+	len -= ETH_FCS_LEN;
 	skb_put(skb, len);
 
 	skb->protocol = eth_type_trans(skb, dev);
@@ -1035,6 +1041,23 @@ static void bfin_mac_rx(struct net_device *dev)
 
 #if defined(BFIN_MAC_CSUM_OFFLOAD)
 	skb->csum = current_rx_ptr->status.ip_payload_csum;
+	/*
+	 * Deduce Ethernet FCS from hardware generated IP payload checksum.
+	 * IP checksum is based on 16-bit one's complement algorithm.
+	 * To deduce a value from checksum is equal to add its inversion.
+	 * If the IP payload len is odd, the inversed FCS should also
+	 * begin from odd address and leave first byte zero.
+	 */
+	if (skb->len % 2) {
+		fcs[0] = 0;
+		for (i = 0; i < ETH_FCS_LEN; i++)
+			fcs[i + 1] = ~skb->data[skb->len + i];
+		skb->csum = csum_partial(fcs, ETH_FCS_LEN + 1, skb->csum);
+	} else {
+		for (i = 0; i < ETH_FCS_LEN; i++)
+			fcs[i] = ~skb->data[skb->len + i];
+		skb->csum = csum_partial(fcs, ETH_FCS_LEN, skb->csum);
+	}
 	skb->ip_summed = CHECKSUM_COMPLETE;
 #endif
 
-- 
1.7.1

[PATCH 05/11] netdev: bfin_mac: clear RXCKS if hardware generated checksum is not enabled

[Mike Frysinger]

From: Sonic Zhang <sonic.zhang@analog.com>

Otherwise we might be get a setting mismatch from a previous module or
bootloader and what the driver currently expects.

Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 drivers/net/bfin_mac.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
index 5b00fc8..6d69bbb 100644
--- a/drivers/net/bfin_mac.c
+++ b/drivers/net/bfin_mac.c
@@ -515,10 +515,11 @@ void setup_system_regs(struct net_device *dev)
 	 * Configure checksum support and rcve frame word alignment
 	 */
 	sysctl = bfin_read_EMAC_SYSCTL();
+	sysctl |= RXDWA;
 #if defined(BFIN_MAC_CSUM_OFFLOAD)
-	sysctl |= RXDWA | RXCKS;
+	sysctl |= RXCKS;
 #else
-	sysctl |= RXDWA;
+	sysctl &= ~RXCKS;
 #endif
 	bfin_write_EMAC_SYSCTL(sysctl);
 
-- 
1.7.1

[PATCH 06/11] netdev: bfin_mac: avoid tx skb overflows in the tx DMA ring

[Mike Frysinger]

From: Sonic Zhang <sonic.zhang@analog.com>

Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 drivers/net/bfin_mac.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
index 6d69bbb..0b5ea01 100644
--- a/drivers/net/bfin_mac.c
+++ b/drivers/net/bfin_mac.c
@@ -920,6 +920,9 @@ static int bfin_mac_hard_start_xmit(struct sk_buff *skb,
 	u32 data_align = (unsigned long)(skb->data) & 0x3;
 	union skb_shared_tx *shtx = skb_tx(skb);
 
+	if (current_tx_ptr->next == tx_list_head)
+		return NETDEV_TX_BUSY;
+
 	current_tx_ptr->skb = skb;
 
 	if (data_align == 0x2) {
-- 
1.7.1

[PATCH 07/11] netdev: bfin_mac: add support for wake-on-lan magic packets

[Mike Frysinger]

From: Michael Hennerich <michael.hennerich@analog.com>

Note that WOL works only in PM Suspend Standby Mode (Sleep Mode).

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 drivers/net/bfin_mac.c |   76 +++++++++++++++++++++++++++++++++++++++++++++--
 drivers/net/bfin_mac.h |    3 ++
 2 files changed, 75 insertions(+), 4 deletions(-)

diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
index 0b5ea01..e80ccaa 100644
--- a/drivers/net/bfin_mac.c
+++ b/drivers/net/bfin_mac.c
@@ -464,6 +464,14 @@ static int mii_probe(struct net_device *dev)
  * Ethtool support
  */
 
+/*
+ * interrupt routine for magic packet wakeup
+ */
+static irqreturn_t bfin_mac_wake_interrupt(int irq, void *dev_id)
+{
+	return IRQ_HANDLED;
+}
+
 static int
 bfin_mac_ethtool_getsettings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
@@ -498,11 +506,57 @@ static void bfin_mac_ethtool_getdrvinfo(struct net_device *dev,
 	strcpy(info->bus_info, dev_name(&dev->dev));
 }
 
+static void bfin_mac_ethtool_getwol(struct net_device *dev,
+	struct ethtool_wolinfo *wolinfo)
+{
+	struct bfin_mac_local *lp = netdev_priv(dev);
+
+	wolinfo->supported = WAKE_MAGIC;
+	wolinfo->wolopts = lp->wol;
+}
+
+static int bfin_mac_ethtool_setwol(struct net_device *dev,
+	struct ethtool_wolinfo *wolinfo)
+{
+	struct bfin_mac_local *lp = netdev_priv(dev);
+	int rc;
+
+	if (wolinfo->wolopts & (WAKE_MAGICSECURE |
+				WAKE_UCAST |
+				WAKE_MCAST |
+				WAKE_BCAST |
+				WAKE_ARP))
+		return -EOPNOTSUPP;
+
+	lp->wol = wolinfo->wolopts;
+
+	if (lp->wol && !lp->irq_wake_requested) {
+		/* register wake irq handler */
+		rc = request_irq(IRQ_MAC_WAKEDET, bfin_mac_wake_interrupt,
+				 IRQF_DISABLED, "EMAC_WAKE", dev);
+		if (rc)
+			return rc;
+		lp->irq_wake_requested = true;
+	}
+
+	if (!lp->wol && lp->irq_wake_requested) {
+		free_irq(IRQ_MAC_WAKEDET, dev);
+		lp->irq_wake_requested = false;
+	}
+
+	/* Make sure the PHY driver doesn't suspend */
+	device_init_wakeup(&dev->dev, lp->wol);
+
+	return 0;
+}
+
 static const struct ethtool_ops bfin_mac_ethtool_ops = {
 	.get_settings = bfin_mac_ethtool_getsettings,
 	.set_settings = bfin_mac_ethtool_setsettings,
 	.get_link = ethtool_op_get_link,
 	.get_drvinfo = bfin_mac_ethtool_getdrvinfo,
+	.get_wol = bfin_mac_ethtool_getwol,
+	.set_wol = bfin_mac_ethtool_setwol,
 };
 
 /**************************************************************************/
@@ -1472,9 +1526,16 @@ static int __devexit bfin_mac_remove(struct platform_device *pdev)
 static int bfin_mac_suspend(struct platform_device *pdev, pm_message_t mesg)
 {
 	struct net_device *net_dev = platform_get_drvdata(pdev);
+	struct bfin_mac_local *lp = netdev_priv(net_dev);
 
-	if (netif_running(net_dev))
-		bfin_mac_close(net_dev);
+	if (lp->wol) {
+		bfin_write_EMAC_OPMODE((bfin_read_EMAC_OPMODE() & ~TE) | RE);
+		bfin_write_EMAC_WKUP_CTL(MPKE);
+		enable_irq_wake(IRQ_MAC_WAKEDET);
+	} else {
+		if (netif_running(net_dev))
+			bfin_mac_close(net_dev);
+	}
 
 	return 0;
 }
@@ -1482,9 +1543,16 @@ static int bfin_mac_suspend(struct platform_device *pdev, pm_message_t mesg)
 static int bfin_mac_resume(struct platform_device *pdev)
 {
 	struct net_device *net_dev = platform_get_drvdata(pdev);
+	struct bfin_mac_local *lp = netdev_priv(net_dev);
 
-	if (netif_running(net_dev))
-		bfin_mac_open(net_dev);
+	if (lp->wol) {
+		bfin_write_EMAC_OPMODE(bfin_read_EMAC_OPMODE() | TE);
+		bfin_write_EMAC_WKUP_CTL(0);
+		disable_irq_wake(IRQ_MAC_WAKEDET);
+	} else {
+		if (netif_running(net_dev))
+			bfin_mac_open(net_dev);
+	}
 
 	return 0;
 }
diff --git a/drivers/net/bfin_mac.h b/drivers/net/bfin_mac.h
index 87c454f..1ae7b82 100644
--- a/drivers/net/bfin_mac.h
+++ b/drivers/net/bfin_mac.h
@@ -66,6 +66,9 @@ struct bfin_mac_local {
 	unsigned char Mac[6];	/* MAC address of the board */
 	spinlock_t lock;
 
+	int wol;		/* Wake On Lan */
+	int irq_wake_requested;
+
 	/* MII and PHY stuffs */
 	int old_link;          /* used by bf537_adjust_link */
 	int old_speed;
-- 
1.7.1

[PATCH 08/11] netdev: bfin_mac: use promiscuous flag for promiscuous mode

[Mike Frysinger]

From: Sonic Zhang <sonic.zhang@analog.com>

Rather than using the Receive All Frames (RAF) bit to enable promiscuous
mode, use the Promiscuous (PR) bit.  This lowers overhead at runtime as
we let the hardware process the packets that should actually be checked.

Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 drivers/net/bfin_mac.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
index e80ccaa..04cda8f 100644
--- a/drivers/net/bfin_mac.c
+++ b/drivers/net/bfin_mac.c
@@ -1274,7 +1274,7 @@ static void bfin_mac_set_multicast_list(struct net_device *dev)
 	if (dev->flags & IFF_PROMISC) {
 		printk(KERN_INFO "%s: set to promisc mode\n", dev->name);
 		sysctl = bfin_read_EMAC_OPMODE();
-		sysctl |= RAF;
+		sysctl |= PR;
 		bfin_write_EMAC_OPMODE(sysctl);
 	} else if (dev->flags & IFF_ALLMULTI) {
 		/* accept all multicast */
-- 
1.7.1

[PATCH 09/11] netdev: bfin_mac: only use hardware checksum in normal IPv4 mode

[Mike Frysinger]

From: Jon Kowal <jon.kowal@dspecialists.de>

The Blackfin on-chip MAC checksum logic only works when the IP packet has
a header length of 20 bytes.  This is true for most IPv4 packets, but not
for IPv6 packets or IPv4 packets which use header options.  So only use
the hardware checksum when appropriate.

Signed-off-by: Jon Kowal <jon.kowal@dspecialists.de>
Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 drivers/net/bfin_mac.c |   42 +++++++++++++++++++++++++-----------------
 1 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
index 04cda8f..00d0f25 100644
--- a/drivers/net/bfin_mac.c
+++ b/drivers/net/bfin_mac.c
@@ -1044,6 +1044,7 @@ out:
 	return NETDEV_TX_OK;
 }
 
+#define IP_HEADER_OFF  0
 #define RX_ERROR_MASK (RX_LONG | RX_ALIGN | RX_CRC | RX_LEN | \
 	RX_FRAG | RX_ADDR | RX_DMAO | RX_PHY | RX_LATE | RX_RANGE)
 
@@ -1098,25 +1099,32 @@ static void bfin_mac_rx(struct net_device *dev)
 	bfin_rx_hwtstamp(dev, skb);
 
 #if defined(BFIN_MAC_CSUM_OFFLOAD)
-	skb->csum = current_rx_ptr->status.ip_payload_csum;
-	/*
-	 * Deduce Ethernet FCS from hardware generated IP payload checksum.
-	 * IP checksum is based on 16-bit one's complement algorithm.
-	 * To deduce a value from checksum is equal to add its inversion.
-	 * If the IP payload len is odd, the inversed FCS should also
-	 * begin from odd address and leave first byte zero.
+	/* Checksum offloading only works for IPv4 packets with the standard IP header
+	 * length of 20 bytes, because the blackfin MAC checksum calculation is
+	 * based on that assumption. We must NOT use the calculated checksum if our
+	 * IP version or header break that assumption.
 	 */
-	if (skb->len % 2) {
-		fcs[0] = 0;
-		for (i = 0; i < ETH_FCS_LEN; i++)
-			fcs[i + 1] = ~skb->data[skb->len + i];
-		skb->csum = csum_partial(fcs, ETH_FCS_LEN + 1, skb->csum);
-	} else {
-		for (i = 0; i < ETH_FCS_LEN; i++)
-			fcs[i] = ~skb->data[skb->len + i];
-		skb->csum = csum_partial(fcs, ETH_FCS_LEN, skb->csum);
+	if (skb->data[IP_HEADER_OFF] == 0x45) {
+		skb->csum = current_rx_ptr->status.ip_payload_csum;
+		/*
+		 * Deduce Ethernet FCS from hardware generated IP payload checksum.
+		 * IP checksum is based on 16-bit one's complement algorithm.
+		 * To deduce a value from checksum is equal to add its inversion.
+		 * If the IP payload len is odd, the inversed FCS should also
+		 * begin from odd address and leave first byte zero.
+		 */
+		if (skb->len % 2) {
+			fcs[0] = 0;
+			for (i = 0; i < ETH_FCS_LEN; i++)
+				fcs[i + 1] = ~skb->data[skb->len + i];
+			skb->csum = csum_partial(fcs, ETH_FCS_LEN + 1, skb->csum);
+		} else {
+			for (i = 0; i < ETH_FCS_LEN; i++)
+				fcs[i] = ~skb->data[skb->len + i];
+			skb->csum = csum_partial(fcs, ETH_FCS_LEN, skb->csum);
+		}
+		skb->ip_summed = CHECKSUM_COMPLETE;
 	}
-	skb->ip_summed = CHECKSUM_COMPLETE;
 #endif
 
 	netif_rx(skb);
-- 
1.7.1

[PATCH 10/11] netdev: bfin_mac: handle timeouts with the MDIO registers gracefully

[Mike Frysinger]

Have the low level MDIO functions pass back up timeout information so we
don't waste time polling them multiple times when there is a problem, and
so we don't let higher layers think the device is available when it isn't.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 drivers/net/bfin_mac.c |   53 ++++++++++++++++++++++++++++++-----------------
 1 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
index 00d0f25..a4c9ab2 100644
--- a/drivers/net/bfin_mac.c
+++ b/drivers/net/bfin_mac.c
@@ -81,9 +81,6 @@ static u16 pin_req[] = P_RMII0;
 static u16 pin_req[] = P_MII0;
 #endif
 
-static void bfin_mac_disable(void);
-static void bfin_mac_enable(void);
-
 static void desc_list_free(void)
 {
 	struct net_dma_desc_rx *r;
@@ -260,7 +257,7 @@ init_error:
  * MII operations
  */
 /* Wait until the previous MDC/MDIO transaction has completed */
-static void bfin_mdio_poll(void)
+static int bfin_mdio_poll(void)
 {
 	int timeout_cnt = MAX_TIMEOUT_CNT;
 
@@ -270,22 +267,30 @@ static void bfin_mdio_poll(void)
 		if (timeout_cnt-- < 0) {
 			printk(KERN_ERR DRV_NAME
 			": wait MDC/MDIO transaction to complete timeout\n");
-			break;
+			return -ETIMEDOUT;
 		}
 	}
+
+	return 0;
 }
 
 /* Read an off-chip register in a PHY through the MDC/MDIO port */
 static int bfin_mdiobus_read(struct mii_bus *bus, int phy_addr, int regnum)
 {
-	bfin_mdio_poll();
+	int ret;
+
+	ret = bfin_mdio_poll();
+	if (ret)
+		return ret;
 
 	/* read mode */
 	bfin_write_EMAC_STAADD(SET_PHYAD((u16) phy_addr) |
 				SET_REGAD((u16) regnum) |
 				STABUSY);
 
-	bfin_mdio_poll();
+	ret = bfin_mdio_poll();
+	if (ret)
+		return ret;
 
 	return (int) bfin_read_EMAC_STADAT();
 }
@@ -294,7 +299,11 @@ static int bfin_mdiobus_read(struct mii_bus *bus, int phy_addr, int regnum)
 static int bfin_mdiobus_write(struct mii_bus *bus, int phy_addr, int regnum,
 			      u16 value)
 {
-	bfin_mdio_poll();
+	int ret;
+
+	ret = bfin_mdio_poll();
+	if (ret)
+		return ret;
 
 	bfin_write_EMAC_STADAT((u32) value);
 
@@ -304,9 +313,7 @@ static int bfin_mdiobus_write(struct mii_bus *bus, int phy_addr, int regnum,
 				STAOP |
 				STABUSY);
 
-	bfin_mdio_poll();
-
-	return 0;
+	return bfin_mdio_poll();
 }
 
 static int bfin_mdiobus_reset(struct mii_bus *bus)
@@ -1184,8 +1191,9 @@ static void bfin_mac_disable(void)
 /*
  * Enable Interrupts, Receive, and Transmit
  */
-static void bfin_mac_enable(void)
+static int bfin_mac_enable(void)
 {
+	int ret;
 	u32 opmode;
 
 	pr_debug("%s: %s\n", DRV_NAME, __func__);
@@ -1195,7 +1203,9 @@ static void bfin_mac_enable(void)
 	bfin_write_DMA1_CONFIG(rx_list_head->desc_a.config);
 
 	/* Wait MII done */
-	bfin_mdio_poll();
+	ret = bfin_mdio_poll();
+	if (ret)
+		return ret;
 
 	/* We enable only RX here */
 	/* ASTP   : Enable Automatic Pad Stripping
@@ -1219,6 +1229,8 @@ static void bfin_mac_enable(void)
 #endif
 	/* Turn on the EMAC rx */
 	bfin_write_EMAC_OPMODE(opmode);
+
+	return 0;
 }
 
 /* Our watchdog timed out. Called by the networking layer */
@@ -1333,7 +1345,7 @@ static void bfin_mac_shutdown(struct net_device *dev)
 static int bfin_mac_open(struct net_device *dev)
 {
 	struct bfin_mac_local *lp = netdev_priv(dev);
-	int retval;
+	int ret;
 	pr_debug("%s: %s\n", dev->name, __func__);
 
 	/*
@@ -1347,18 +1359,21 @@ static int bfin_mac_open(struct net_device *dev)
 	}
 
 	/* initial rx and tx list */
-	retval = desc_list_init();
-
-	if (retval)
-		return retval;
+	ret = desc_list_init();
+	if (ret)
+		return ret;
 
 	phy_start(lp->phydev);
 	phy_write(lp->phydev, MII_BMCR, BMCR_RESET);
 	setup_system_regs(dev);
 	setup_mac_addr(dev->dev_addr);
+
 	bfin_mac_disable();
-	bfin_mac_enable();
+	ret = bfin_mac_enable();
+	if (ret)
+		return ret;
 	pr_debug("hardware init finished\n");
+
 	netif_start_queue(dev);
 	netif_carrier_on(dev);
 
-- 
1.7.1

[PATCH 11/11] netdev: bfin_mac: check for mii_bus platform data

[Mike Frysinger]

From: Sonic Zhang <sonic.zhang@analog.com>

If the platform data for the mii_bus is missing, gracefully error out
rather than deference NULL pointers.

Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 drivers/net/bfin_mac.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
index a4c9ab2..c65d5e1 100644
--- a/drivers/net/bfin_mac.c
+++ b/drivers/net/bfin_mac.c
@@ -1472,6 +1472,11 @@ static int __devinit bfin_mac_probe(struct platform_device *pdev)
 	}
 	pd = pdev->dev.platform_data;
 	lp->mii_bus = platform_get_drvdata(pd);
+	if (!lp->mii_bus) {
+		dev_err(&pdev->dev, "Cannot get mii_bus!\n");
+		rc = -ENODEV;
+		goto out_err_mii_bus_probe;
+	}
 	lp->mii_bus->priv = ndev;
 
 	rc = mii_probe(ndev);
@@ -1517,6 +1522,7 @@ out_err_request_irq:
 out_err_mii_probe:
 	mdiobus_unregister(lp->mii_bus);
 	mdiobus_free(lp->mii_bus);
+out_err_mii_bus_probe:
 	peripheral_free_list(pin_req);
 out_err_probe_mac:
 	platform_set_drvdata(pdev, NULL);
-- 
1.7.1

Re: [PATCH 06/11] netdev: bfin_mac: avoid tx skb overflows in the tx DMA ring

[David Miller]

From: Mike Frysinger <vapier@gentoo.org>
Date: Sun,  9 May 2010 06:18:52 -0400

> From: Sonic Zhang <sonic.zhang@analog.com>
> 
> Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

This should never ever happen, it's a bug and you should print a warning
message when and if it does actually occur.

At any point where your ->next pointer hits tx_list_head, the queue
should have been stopped by your driver and therefore the networking
core will never pass another packet to you.

If this condition is actually triggering, it means you're not locking
properly or you have some race.

> ---
>  drivers/net/bfin_mac.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
> index 6d69bbb..0b5ea01 100644
> --- a/drivers/net/bfin_mac.c
> +++ b/drivers/net/bfin_mac.c
> @@ -920,6 +920,9 @@ static int bfin_mac_hard_start_xmit(struct sk_buff *skb,
>  	u32 data_align = (unsigned long)(skb->data) & 0x3;
>  	union skb_shared_tx *shtx = skb_tx(skb);
>  
> +	if (current_tx_ptr->next == tx_list_head)
> +		return NETDEV_TX_BUSY;
> +
>  	current_tx_ptr->skb = skb;
>  
>  	if (data_align == 0x2) {
> -- 
> 1.7.1
> 

Re: [PATCH 09/11] netdev: bfin_mac: only use hardware checksum in normal IPv4 mode

[David Miller]

From: Mike Frysinger <vapier@gentoo.org>
Date: Sun,  9 May 2010 06:18:55 -0400

> From: Jon Kowal <jon.kowal@dspecialists.de>
> 
> The Blackfin on-chip MAC checksum logic only works when the IP packet has
> a header length of 20 bytes.  This is true for most IPv4 packets, but not
> for IPv6 packets or IPv4 packets which use header options.  So only use
> the hardware checksum when appropriate.
> 
> Signed-off-by: Jon Kowal <jon.kowal@dspecialists.de>
> Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

In order to make the tree bisectable, combine bug fixes into the changes
that add the bugs instead of letting them live in the final tree when
they don't need to.

You should have just combined this patch into the patch that added the
FCS inversion stuff.

You're going to need to respin and repost this entire patch set after
fixing all of these problems I've found.

Thanks.

Re: [Uclinux-dist-devel] [PATCH 09/11] netdev: bfin_mac: only use hardware checksum in normal IPv4 mode

[Mike Frysinger]

On Mon, May 10, 2010 at 07:42, David Miller wrote:
> From: Mike Frysinger <vapier@gentoo.org>
> Date: Sun,  9 May 2010 06:18:55 -0400
>
>> From: Jon Kowal <jon.kowal@dspecialists.de>
>>
>> The Blackfin on-chip MAC checksum logic only works when the IP packet has
>> a header length of 20 bytes.  This is true for most IPv4 packets, but not
>> for IPv6 packets or IPv4 packets which use header options.  So only use
>> the hardware checksum when appropriate.
>>
>> Signed-off-by: Jon Kowal <jon.kowal@dspecialists.de>
>> Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
>> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>
> In order to make the tree bisectable, combine bug fixes into the changes
> that add the bugs instead of letting them live in the final tree when
> they don't need to.
>
> You should have just combined this patch into the patch that added the
> FCS inversion stuff.

i dont think that is true.  this bug existed before the FCS inversion
stuff -- notice the csum complete behavior changes.
-mike

Re: [PATCH 06/11] netdev: bfin_mac: avoid tx skb overflows in the tx DMA ring

[David Miller]

From: Sonic Zhang <sonic.adi@gmail.com>
Date: Tue, 18 May 2010 18:52:17 +0800

[ Please never drop the mailing list when you're trying to
  discuss something networking related with me, thanks.
  I've put the CC: back. ]

> On Mon, May 10, 2010 at 7:40 PM, David Miller <davem@davemloft.net> wrote:
>> From: Mike Frysinger <vapier@gentoo.org>
>> Date: Sun,  9 May 2010 06:18:52 -0400
>>
>>> From: Sonic Zhang <sonic.zhang@analog.com>
>>>
>>> Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
>>> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>>
>> This should never ever happen, it's a bug and you should print a warning
>> message when and if it does actually occur.
>>
>> At any point where your ->next pointer hits tx_list_head, the queue
>> should have been stopped by your driver and therefore the networking
>> core will never pass another packet to you.
> 
> To reduce the tx output overhead, the tx interrupt is not enabled and
> handled in this driver. So, the driver doesn't know when to restart
> the tx queue if the queue is stopped in ndo_start_xmit() at the point
> where next pointer hits tx_list_head.
> 
> In this case, although TX buffer list full rarely happens, the check
> is still a safeguard.

You can't do that, if you don't use the TX interrupt then you can leave
SKBs stale in your TX ring for indefinite periods of time which is
illegal.

SKBs hold onto resources that can't be held indefinitely, such as TCP
socket references and netfilter conntrack state.  So if you leave a
packet in your TX ring for a long time, there might be a TCP socket
that now cannot be closed and freed up because of that.

You must therefore free them very as soon as possible after the
hardware is done with them.

Re: [PATCH 06/11] netdev: bfin_mac: avoid tx skb overflows in the tx DMA ring

[Sonic Zhang]

On Wed, May 19, 2010 at 3:29 AM, David Miller <davem@davemloft.net> wrote:
> From: Sonic Zhang <sonic.adi@gmail.com>
> Date: Tue, 18 May 2010 18:52:17 +0800
>
> [ Please never drop the mailing list when you're trying to
>  discuss something networking related with me, thanks.
>  I've put the CC: back. ]
>
>> On Mon, May 10, 2010 at 7:40 PM, David Miller <davem@davemloft.net> wrote:
>>> From: Mike Frysinger <vapier@gentoo.org>
>>> Date: Sun,  9 May 2010 06:18:52 -0400
>>>
>>>> From: Sonic Zhang <sonic.zhang@analog.com>
>>>>
>>>> Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
>>>> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>>>
>>> This should never ever happen, it's a bug and you should print a warning
>>> message when and if it does actually occur.
>>>
>>> At any point where your ->next pointer hits tx_list_head, the queue
>>> should have been stopped by your driver and therefore the networking
>>> core will never pass another packet to you.
>>
>> To reduce the tx output overhead, the tx interrupt is not enabled and
>> handled in this driver. So, the driver doesn't know when to restart
>> the tx queue if the queue is stopped in ndo_start_xmit() at the point
>> where next pointer hits tx_list_head.
>>
>> In this case, although TX buffer list full rarely happens, the check
>> is still a safeguard.
>
> You can't do that, if you don't use the TX interrupt then you can leave
> SKBs stale in your TX ring for indefinite periods of time which is
> illegal.

No, this doesn't happen, because before ndo_start_xmit() returns, the
old TX buffers and skbs in the ring, which finished DMA operation, are
freed. The only difference is that the free operation of a skb is done
in next tx transfer.

Sonic

>
> SKBs hold onto resources that can't be held indefinitely, such as TCP
> socket references and netfilter conntrack state.  So if you leave a
> packet in your TX ring for a long time, there might be a TCP socket
> that now cannot be closed and freed up because of that.
>
> You must therefore free them very as soon as possible after the
> hardware is done with them.
>

Re: [PATCH 06/11] netdev: bfin_mac: avoid tx skb overflows in the tx DMA ring

[David Miller]

From: Sonic Zhang <sonic.adi@gmail.com>
Date: Wed, 19 May 2010 17:23:16 +0800

> No, this doesn't happen, because before ndo_start_xmit() returns, the
> old TX buffers and skbs in the ring, which finished DMA operation, are
> freed. The only difference is that the free operation of a skb is done
> in next tx transfer.

This is still illegal.

What if TX activity stops right then, and there is no "next tx
transfer"?

That SKB will never get freed, ever.

You have to fix this.

Re: [PATCH 06/11] netdev: bfin_mac: avoid tx skb overflows in the tx DMA ring

[Sonic Zhang]

On Thu, May 20, 2010 at 6:08 PM, David Miller <davem@davemloft.net> wrote:
> From: Sonic Zhang <sonic.adi@gmail.com>
> Date: Thu, 20 May 2010 17:38:07 +0800
>
>> On Thu, May 20, 2010 at 4:12 AM, David Miller <davem@davemloft.net> wrote:
>>> From: Sonic Zhang <sonic.adi@gmail.com>
>>> Date: Wed, 19 May 2010 17:23:16 +0800
>>>
>>>> No, this doesn't happen, because before ndo_start_xmit() returns, the
>>>> old TX buffers and skbs in the ring, which finished DMA operation, are
>>>> freed. The only difference is that the free operation of a skb is done
>>>> in next tx transfer.
>>>
>>> This is still illegal.
>>>
>>> What if TX activity stops right then, and there is no "next tx
>>> transfer"?
>>>
>> The skb remain in the TX ring will be freed finally when ndo_stop() is
>> called to shutdown the network. So, this is not a problem.
>
> You really don't understand me, and I'm starting to get really
> frustrated.  You must free all packets in your TX ring in a very
> small, finite, amount of time.  This is not optional.  And this
> must happen regardless of what TX traffic which occurs in the future,
> that means it must happen even if TX traffic suddenly stops.
>
> Your driver's behavior is absolutely not acceptable.
>
> Leaving the SKB In the TX ring like that means that potentially there
> is a socket in the system or other major resource that cannot be released
> and freed up.
>
> Please stop your driver from keeping packets in the TX ring indefinitely.
>

Forgot to CC netdev mailing list in my last reply.
Try again.

Sonic

Re: [PATCH 06/11] netdev: bfin_mac: avoid tx skb overflows in the tx DMA ring

[Sonic Zhang]

On Thu, May 20, 2010 at 6:08 PM, David Miller <davem@davemloft.net> wrote:
> From: Sonic Zhang <sonic.adi@gmail.com>
> Date: Thu, 20 May 2010 17:38:07 +0800
>
>> On Thu, May 20, 2010 at 4:12 AM, David Miller <davem@davemloft.net> wrote:
>>> From: Sonic Zhang <sonic.adi@gmail.com>
>>> Date: Wed, 19 May 2010 17:23:16 +0800
>>>
>>>> No, this doesn't happen, because before ndo_start_xmit() returns, the
>>>> old TX buffers and skbs in the ring, which finished DMA operation, are
>>>> freed. The only difference is that the free operation of a skb is done
>>>> in next tx transfer.
>>>
>>> This is still illegal.
>>>
>>> What if TX activity stops right then, and there is no "next tx
>>> transfer"?
>>>
>> The skb remain in the TX ring will be freed finally when ndo_stop() is
>> called to shutdown the network. So, this is not a problem.
>
> You really don't understand me, and I'm starting to get really
> frustrated.  You must free all packets in your TX ring in a very
> small, finite, amount of time.  This is not optional.  And this
> must happen regardless of what TX traffic which occurs in the future,
> that means it must happen even if TX traffic suddenly stops.
>

OK. I didn't figure out that the socket may not be closed if its skbs
stay active somewhere in system for a long time. Thanks for your
explanation. I will send a new patch to enabled tx interrupt and free
skb from tx ring immediately.


Sonic

> Your driver's behavior is absolutely not acceptable.
>
> Leaving the SKB In the TX ring like that means that potentially there
> is a socket in the system or other major resource that cannot be released
> and freed up.
>
> Please stop your driver from keeping packets in the TX ring indefinitely.
>