From ccea2968398c959493cdce503ae94206d2026fbe Mon Sep 17 00:00:00 2001 From: Russell King Date: Tue, 8 Jul 2014 13:01:38 +0100 Subject: [PATCH 1/7] net: fec: better implementation of iMX6 ERR006358 quirk Using a (delayed) workqueue for ERR006358 is not correct - a work queue is a single-trigger device. Once the work queue has been scheduled, it can't be re-scheduled until it has been run. This can cause problems - with an appropriate packet timing, we can end up with packets queued, but not sent by the hardware, resulting in the transmit timeout firing. Re-implement this as per the workaround detailed in the ERR006358 documentation - if there are packets waiting to be sent when we service the transmit ring, and we see that the transmitter is not running, kick the transmitter to run the pending entries in the ring. Testing here with a 10Mbit half duplex link sees the resulting iperf TCP bandwidth increase from between 1 to 2Mbps to between 8 to 9Mbps. Acked-by: Fugang Duan Signed-off-by: Russell King Signed-off-by: David S. Miller --- drivers/net/ethernet/freescale/fec.h | 1 - drivers/net/ethernet/freescale/fec_main.c | 30 +++-------------------- 2 files changed, 4 insertions(+), 27 deletions(-) diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h index 96d2a18f1b99..17e294970207 100644 --- a/drivers/net/ethernet/freescale/fec.h +++ b/drivers/net/ethernet/freescale/fec.h @@ -259,7 +259,6 @@ struct bufdesc_ex { struct fec_enet_delayed_work { struct delayed_work delay_work; bool timeout; - bool trig_tx; }; /* The FEC buffer descriptors track the ring buffers. The rx_bd_base and diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 9d82d915b06d..8cfc3a3de7fb 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -342,22 +342,6 @@ fec_enet_clear_csum(struct sk_buff *skb, struct net_device *ndev) return 0; } -static void -fec_enet_submit_work(struct bufdesc *bdp, struct fec_enet_private *fep) -{ - const struct platform_device_id *id_entry = - platform_get_device_id(fep->pdev); - struct bufdesc *bdp_pre; - - bdp_pre = fec_enet_get_prevdesc(bdp, fep); - if ((id_entry->driver_data & FEC_QUIRK_ERR006358) && - !(bdp_pre->cbd_sc & BD_ENET_TX_READY)) { - fep->delay_work.trig_tx = true; - schedule_delayed_work(&(fep->delay_work.delay_work), - msecs_to_jiffies(1)); - } -} - static int fec_enet_txq_submit_frag_skb(struct sk_buff *skb, struct net_device *ndev) { @@ -545,8 +529,6 @@ static int fec_enet_txq_submit_skb(struct sk_buff *skb, struct net_device *ndev) status |= (BD_ENET_TX_READY | BD_ENET_TX_TC); bdp->cbd_sc = status; - fec_enet_submit_work(bdp, fep); - /* If this was the last BD in the ring, start at the beginning again. */ bdp = fec_enet_get_nextdesc(last_bdp, fep); @@ -735,8 +717,6 @@ static int fec_enet_txq_submit_tso(struct sk_buff *skb, struct net_device *ndev) /* Save skb pointer */ fep->tx_skbuff[index] = skb; - fec_enet_submit_work(bdp, fep); - skb_tx_timestamp(skb); fep->cur_tx = bdp; @@ -1065,11 +1045,6 @@ static void fec_enet_work(struct work_struct *work) } rtnl_unlock(); } - - if (fep->delay_work.trig_tx) { - fep->delay_work.trig_tx = false; - writel(0, fep->hwp + FEC_X_DES_ACTIVE); - } } static void @@ -1166,7 +1141,10 @@ fec_enet_tx(struct net_device *ndev) netif_wake_queue(ndev); } } - return; + + /* ERR006538: Keep the transmitter going */ + if (bdp != fep->cur_tx && readl(fep->hwp + FEC_X_DES_ACTIVE) == 0) + writel(0, fep->hwp + FEC_X_DES_ACTIVE); } /* During a receive, the cur_rx points to the current incoming buffer. From 36cdc743a320e78a5d12ca9765ec0f7d9f07b1f5 Mon Sep 17 00:00:00 2001 From: Russell King Date: Tue, 8 Jul 2014 13:01:44 +0100 Subject: [PATCH 2/7] net: fec: replace delayed work with standard work As of "better implementation of iMX6 ERR006358 quirk", we no longer have a requirement for a delayed work. Moreover, the work is now only used for timeout purposes, so the timeout flag is also pointless - we set it each time we queue the work, and the work clears it. Replace the fec_enet_delayed_work struct with a standard work_struct, resulting in simplified timeout handling code. Acked-by: Fugang Duan Signed-off-by: Russell King Signed-off-by: David S. Miller --- drivers/net/ethernet/freescale/fec.h | 8 ++---- drivers/net/ethernet/freescale/fec_main.c | 34 ++++++++++------------- 2 files changed, 16 insertions(+), 26 deletions(-) diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h index 17e294970207..bd53caf1c1eb 100644 --- a/drivers/net/ethernet/freescale/fec.h +++ b/drivers/net/ethernet/freescale/fec.h @@ -256,11 +256,6 @@ struct bufdesc_ex { #define FLAG_RX_CSUM_ENABLED (BD_ENET_RX_ICE | BD_ENET_RX_PCR) #define FLAG_RX_CSUM_ERROR (BD_ENET_RX_ICE | BD_ENET_RX_PCR) -struct fec_enet_delayed_work { - struct delayed_work delay_work; - bool timeout; -}; - /* The FEC buffer descriptors track the ring buffers. The rx_bd_base and * tx_bd_base always point to the base of the buffer descriptors. The * cur_rx and cur_tx point to the currently available buffer. @@ -326,6 +321,8 @@ struct fec_enet_private { struct napi_struct napi; int csum_flags; + struct work_struct tx_timeout_work; + struct ptp_clock *ptp_clock; struct ptp_clock_info ptp_caps; unsigned long last_overflow_check; @@ -338,7 +335,6 @@ struct fec_enet_private { int hwts_rx_en; int hwts_tx_en; struct timer_list time_keep; - struct fec_enet_delayed_work delay_work; struct regulator *reg_phy; }; diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 8cfc3a3de7fb..b2ae7e706d5e 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -1020,31 +1020,25 @@ fec_timeout(struct net_device *ndev) ndev->stats.tx_errors++; - fep->delay_work.timeout = true; - schedule_delayed_work(&(fep->delay_work.delay_work), 0); + schedule_work(&fep->tx_timeout_work); } -static void fec_enet_work(struct work_struct *work) +static void fec_enet_timeout_work(struct work_struct *work) { struct fec_enet_private *fep = - container_of(work, - struct fec_enet_private, - delay_work.delay_work.work); + container_of(work, struct fec_enet_private, tx_timeout_work); struct net_device *ndev = fep->netdev; - if (fep->delay_work.timeout) { - fep->delay_work.timeout = false; - rtnl_lock(); - if (netif_device_present(ndev) || netif_running(ndev)) { - napi_disable(&fep->napi); - netif_tx_lock_bh(ndev); - fec_restart(ndev); - netif_wake_queue(ndev); - netif_tx_unlock_bh(ndev); - napi_enable(&fep->napi); - } - rtnl_unlock(); + rtnl_lock(); + if (netif_device_present(ndev) || netif_running(ndev)) { + napi_disable(&fep->napi); + netif_tx_lock_bh(ndev); + fec_restart(ndev); + netif_wake_queue(ndev); + netif_tx_unlock_bh(ndev); + napi_enable(&fep->napi); } + rtnl_unlock(); } static void @@ -2642,7 +2636,7 @@ fec_probe(struct platform_device *pdev) if (fep->bufdesc_ex && fep->ptp_clock) netdev_info(ndev, "registered PHC device %d\n", fep->dev_id); - INIT_DELAYED_WORK(&(fep->delay_work.delay_work), fec_enet_work); + INIT_WORK(&fep->tx_timeout_work, fec_enet_timeout_work); return 0; failed_register: @@ -2667,7 +2661,7 @@ fec_drv_remove(struct platform_device *pdev) struct net_device *ndev = platform_get_drvdata(pdev); struct fec_enet_private *fep = netdev_priv(ndev); - cancel_delayed_work_sync(&(fep->delay_work.delay_work)); + cancel_work_sync(&fep->tx_timeout_work); unregister_netdev(ndev); fec_enet_mii_remove(fep); del_timer_sync(&fep->time_keep); From db3421c114cfa6326861bc95e604785f4c64293b Mon Sep 17 00:00:00 2001 From: Russell King Date: Tue, 8 Jul 2014 13:01:49 +0100 Subject: [PATCH 3/7] net: fec: clear receive interrupts before processing a packet Clear any pending receive interrupt before we process a pending packet. This helps to avoid any spurious interrupts being raised after we have fully cleaned the receive ring, while still allowing an interrupt to be raised if we receive another packet. The position of this is critical: we must do this prior to reading the next packet status to avoid potentially dropping an interrupt when a packet is still pending. Acked-by: Fugang Duan Signed-off-by: Russell King Signed-off-by: David S. Miller --- drivers/net/ethernet/freescale/fec_main.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index b2ae7e706d5e..79d578d6db8a 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -1184,6 +1184,8 @@ fec_enet_rx(struct net_device *ndev, int budget) if ((status & BD_ENET_RX_LAST) == 0) netdev_err(ndev, "rcv is not +last\n"); + writel(FEC_ENET_RXF, fep->hwp + FEC_IEVENT); + /* Check for errors. */ if (status & (BD_ENET_RX_LG | BD_ENET_RX_SH | BD_ENET_RX_NO | BD_ENET_RX_CR | BD_ENET_RX_OV)) { From c1d7c48ff769021fab0591cfe331c0061186cb31 Mon Sep 17 00:00:00 2001 From: Russell King Date: Tue, 8 Jul 2014 13:01:54 +0100 Subject: [PATCH 4/7] net: fec: reorder ethtool ops to match order in struct declaration This allows us to merge two separate preprocessor conditionals together. Acked-by: Fugang Duan Signed-off-by: Russell King Signed-off-by: David S. Miller --- drivers/net/ethernet/freescale/fec_main.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 79d578d6db8a..c6b1bf797a39 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -2014,21 +2014,19 @@ static int fec_enet_nway_reset(struct net_device *dev) } static const struct ethtool_ops fec_enet_ethtool_ops = { -#if !defined(CONFIG_M5272) - .get_pauseparam = fec_enet_get_pauseparam, - .set_pauseparam = fec_enet_set_pauseparam, -#endif .get_settings = fec_enet_get_settings, .set_settings = fec_enet_set_settings, .get_drvinfo = fec_enet_get_drvinfo, - .get_link = ethtool_op_get_link, - .get_ts_info = fec_enet_get_ts_info, .nway_reset = fec_enet_nway_reset, + .get_link = ethtool_op_get_link, #ifndef CONFIG_M5272 - .get_ethtool_stats = fec_enet_get_ethtool_stats, + .get_pauseparam = fec_enet_get_pauseparam, + .set_pauseparam = fec_enet_set_pauseparam, .get_strings = fec_enet_get_strings, + .get_ethtool_stats = fec_enet_get_ethtool_stats, .get_sset_count = fec_enet_get_sset_count, #endif + .get_ts_info = fec_enet_get_ts_info, }; static int fec_enet_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd) From 344756f6e36b056ed361eedbd68244b108bdb1c6 Mon Sep 17 00:00:00 2001 From: Russell King Date: Tue, 8 Jul 2014 13:01:59 +0100 Subject: [PATCH 5/7] net: fec: add support for dumping transmit ring on timeout When we timeout on transmit, it would be useful to dump the transmit ring, so we can see the ring state. This can be helpful to diagnose the cause of transmit timeouts. Acked-by: Fugang Duan Signed-off-by: Russell King Signed-off-by: David S. Miller --- drivers/net/ethernet/freescale/fec_main.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index c6b1bf797a39..09fcdc768931 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -320,6 +320,27 @@ static void *swap_buffer(void *bufaddr, int len) return bufaddr; } +static void fec_dump(struct net_device *ndev) +{ + struct fec_enet_private *fep = netdev_priv(ndev); + struct bufdesc *bdp = fep->tx_bd_base; + unsigned int index = 0; + + netdev_info(ndev, "TX ring dump\n"); + pr_info("Nr SC addr len SKB\n"); + + do { + pr_info("%3u %c%c 0x%04x 0x%08lx %4u %p\n", + index, + bdp == fep->cur_tx ? 'S' : ' ', + bdp == fep->dirty_tx ? 'H' : ' ', + bdp->cbd_sc, bdp->cbd_bufaddr, bdp->cbd_datlen, + fep->tx_skbuff[index]); + bdp = fec_enet_get_nextdesc(bdp, fep); + index++; + } while (bdp != fep->tx_bd_base); +} + static inline bool is_ipv4_pkt(struct sk_buff *skb) { return skb->protocol == htons(ETH_P_IP) && ip_hdr(skb)->version == 4; @@ -1018,6 +1039,8 @@ fec_timeout(struct net_device *ndev) { struct fec_enet_private *fep = netdev_priv(ndev); + fec_dump(ndev); + ndev->stats.tx_errors++; schedule_work(&fep->tx_timeout_work); From 96018f52c532b69424d354a848ac7812136650f6 Mon Sep 17 00:00:00 2001 From: Russell King Date: Tue, 8 Jul 2014 13:02:04 +0100 Subject: [PATCH 6/7] net: fec: remove useless status check in tx reap path Remove a useless status check in the transmit reap path - we have already checked that the BD_ENET_TX_READY bit is clear, and as the hardware only ever clears this bit, there is no way this test can ever be true. Acked-by: Fugang Duan Signed-off-by: Russell King Signed-off-by: David S. Miller --- drivers/net/ethernet/freescale/fec_main.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 09fcdc768931..8679c919419c 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -1133,9 +1133,6 @@ fec_enet_tx(struct net_device *ndev) skb_tstamp_tx(skb, &shhwtstamps); } - if (status & BD_ENET_TX_READY) - netdev_err(ndev, "HEY! Enet xmit interrupt and TX_READY\n"); - /* Deferred means some collisions occurred during transmit, * but we eventually sent the packet OK. */ From bfd4ecdd87d350e19457fe0d02fa1e046774c44e Mon Sep 17 00:00:00 2001 From: Russell King Date: Tue, 8 Jul 2014 13:02:09 +0100 Subject: [PATCH 7/7] net: fec: consolidate hwtstamp implementation Both transmit and receive use the same infrastructure for calculating the packet timestamp. Rather than duplicating the code, provide a function to do this common work. Model this function in the Intel e1000e version which avoids calling ns_to_ktime() within the spinlock; the spinlock is critical for timecounter_cyc2time() but not ns_to_ktime(). Acked-by: Richard Cochran Acked-by: Fugang Duan Signed-off-by: Russell King Signed-off-by: David S. Miller --- drivers/net/ethernet/freescale/fec_main.c | 37 ++++++++++++----------- 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 8679c919419c..e0efb212223f 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -1064,6 +1064,21 @@ static void fec_enet_timeout_work(struct work_struct *work) rtnl_unlock(); } +static void +fec_enet_hwtstamp(struct fec_enet_private *fep, unsigned ts, + struct skb_shared_hwtstamps *hwtstamps) +{ + unsigned long flags; + u64 ns; + + spin_lock_irqsave(&fep->tmreg_lock, flags); + ns = timecounter_cyc2time(&fep->tc, ts); + spin_unlock_irqrestore(&fep->tmreg_lock, flags); + + memset(hwtstamps, 0, sizeof(*hwtstamps)); + hwtstamps->hwtstamp = ns_to_ktime(ns); +} + static void fec_enet_tx(struct net_device *ndev) { @@ -1122,14 +1137,9 @@ fec_enet_tx(struct net_device *ndev) if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) && fep->bufdesc_ex) { struct skb_shared_hwtstamps shhwtstamps; - unsigned long flags; struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp; - memset(&shhwtstamps, 0, sizeof(shhwtstamps)); - spin_lock_irqsave(&fep->tmreg_lock, flags); - shhwtstamps.hwtstamp = ns_to_ktime( - timecounter_cyc2time(&fep->tc, ebdp->ts)); - spin_unlock_irqrestore(&fep->tmreg_lock, flags); + fec_enet_hwtstamp(fep, ebdp->ts, &shhwtstamps); skb_tstamp_tx(skb, &shhwtstamps); } @@ -1288,18 +1298,9 @@ fec_enet_rx(struct net_device *ndev, int budget) skb->protocol = eth_type_trans(skb, ndev); /* Get receive timestamp from the skb */ - if (fep->hwts_rx_en && fep->bufdesc_ex) { - struct skb_shared_hwtstamps *shhwtstamps = - skb_hwtstamps(skb); - unsigned long flags; - - memset(shhwtstamps, 0, sizeof(*shhwtstamps)); - - spin_lock_irqsave(&fep->tmreg_lock, flags); - shhwtstamps->hwtstamp = ns_to_ktime( - timecounter_cyc2time(&fep->tc, ebdp->ts)); - spin_unlock_irqrestore(&fep->tmreg_lock, flags); - } + if (fep->hwts_rx_en && fep->bufdesc_ex) + fec_enet_hwtstamp(fep, ebdp->ts, + skb_hwtstamps(skb)); if (fep->bufdesc_ex && (fep->csum_flags & FLAG_RX_CSUM_ENABLED)) {