ftgmac100: Cleanup tx queue handling

We have a private lock which isn't terribly useful, and we maintain
a "tx_pending" counter for information that's already available
via a trivial arithmetic operation. Then we unconditionaly wake
the queue even when not stopped. Finally our code in tx isn't
really safe vs. a concurrent reclaim. The aspeed chips aren't SMP
today but I prefer the code being right and future proof.

So rip that out and replace it with more "standard" queue handling,
currently with a threshold of 1 queue element, which will be
increased when we implement fragmented sends.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index b4d84e753187e71b298376cc1e319e6a9333f8d6..add7462bf67df93127bbf8bcef1dd3629b39d10c 100644 (file)
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -46,6 +46,9 @@
 #define MAX_PKT_SIZE           1536
 #define RX_BUF_SIZE            MAX_PKT_SIZE    /* must be smaller than 0x3fff */
 
+/* Min number of tx ring entries before stopping queue */
+#define TX_THRESHOLD           (1)
+
 struct ftgmac100_descs {
        struct ftgmac100_rxdes rxdes[RX_QUEUE_ENTRIES];
        struct ftgmac100_txdes txdes[TX_QUEUE_ENTRIES];
@@ -68,9 +71,7 @@ struct ftgmac100 {
        struct sk_buff *tx_skbs[TX_QUEUE_ENTRIES];
        unsigned int tx_clean_pointer;
        unsigned int tx_pointer;
-       unsigned int tx_pending;
        u32 txdes0_edotr_mask;
-       spinlock_t tx_lock;
 
        /* Scratch page to use when rx skb alloc fails */
        void *rx_scratch;
@@ -165,7 +166,6 @@ static int ftgmac100_reset_and_config_mac(struct ftgmac100 *priv)
        priv->rx_pointer = 0;
        priv->tx_clean_pointer = 0;
        priv->tx_pointer = 0;
-       priv->tx_pending = 0;
 
        /* The doc says reset twice with 10us interval */
        if (ftgmac100_reset_mac(priv, maccr))
@@ -551,6 +551,23 @@ static int ftgmac100_next_tx_pointer(int pointer)
        return (pointer + 1) & (TX_QUEUE_ENTRIES - 1);
 }
 
+static u32 ftgmac100_tx_buf_avail(struct ftgmac100 *priv)
+{
+       /* Returns the number of available slots in the TX queue
+        *
+        * This always leaves one free slot so we don't have to
+        * worry about empty vs. full, and this simplifies the
+        * test for ftgmac100_tx_buf_cleanable() below
+        */
+       return (priv->tx_clean_pointer - priv->tx_pointer - 1) &
+               (TX_QUEUE_ENTRIES - 1);
+}
+
+static bool ftgmac100_tx_buf_cleanable(struct ftgmac100 *priv)
+{
+       return priv->tx_pointer != priv->tx_clean_pointer;
+}
+
 static bool ftgmac100_tx_complete_packet(struct ftgmac100 *priv)
 {
        struct net_device *netdev = priv->netdev;
@@ -559,9 +576,6 @@ static bool ftgmac100_tx_complete_packet(struct ftgmac100 *priv)
        struct sk_buff *skb;
        dma_addr_t map;
 
-       if (priv->tx_pending == 0)
-               return false;
-
        pointer = priv->tx_clean_pointer;
        txdes = &priv->descs->txdes[pointer];
 
@@ -583,18 +597,31 @@ static bool ftgmac100_tx_complete_packet(struct ftgmac100 *priv)
 
        priv->tx_clean_pointer = ftgmac100_next_tx_pointer(pointer);
 
-       spin_lock(&priv->tx_lock);
-       priv->tx_pending--;
-       spin_unlock(&priv->tx_lock);
-       netif_wake_queue(netdev);
-
        return true;
 }
 
 static void ftgmac100_tx_complete(struct ftgmac100 *priv)
 {
-       while (ftgmac100_tx_complete_packet(priv))
+       struct net_device *netdev = priv->netdev;
+
+       /* Process all completed packets */
+       while (ftgmac100_tx_buf_cleanable(priv) &&
+              ftgmac100_tx_complete_packet(priv))
                ;
+
+       /* Restart queue if needed */
+       smp_mb();
+       if (unlikely(netif_queue_stopped(netdev) &&
+                    ftgmac100_tx_buf_avail(priv) >= TX_THRESHOLD)) {
+               struct netdev_queue *txq;
+
+               txq = netdev_get_tx_queue(netdev, 0);
+               __netif_tx_lock(txq, smp_processor_id());
+               if (netif_queue_stopped(netdev) &&
+                   ftgmac100_tx_buf_avail(priv) >= TX_THRESHOLD)
+                       netif_wake_queue(netdev);
+               __netif_tx_unlock(txq);
+       }
 }
 
 static int ftgmac100_hard_start_xmit(struct sk_buff *skb,
@@ -652,17 +679,22 @@ static int ftgmac100_hard_start_xmit(struct sk_buff *skb,
                }
        }
 
+       ftgmac100_txdes_set_dma_own(txdes);
+
        /* Update next TX pointer */
        priv->tx_pointer = ftgmac100_next_tx_pointer(pointer);
 
-       spin_lock(&priv->tx_lock);
-       priv->tx_pending++;
-       if (priv->tx_pending == TX_QUEUE_ENTRIES)
+       /* If there isn't enough room for all the fragments of a new packet
+        * in the TX ring, stop the queue. The sequence below is race free
+        * vs. a concurrent restart in ftgmac100_poll()
+        */
+       if (unlikely(ftgmac100_tx_buf_avail(priv) < TX_THRESHOLD)) {
                netif_stop_queue(netdev);
-
-       /* start transmit */
-       ftgmac100_txdes_set_dma_own(txdes);
-       spin_unlock(&priv->tx_lock);
+               /* Order the queue stop with the test below */
+               smp_mb();
+               if (ftgmac100_tx_buf_avail(priv) >= TX_THRESHOLD)
+                       netif_wake_queue(netdev);
+       }
 
        ftgmac100_txdma_normal_prio_start_polling(priv);
 
@@ -980,17 +1012,17 @@ static bool ftgmac100_check_rx(struct ftgmac100 *priv)
 static int ftgmac100_poll(struct napi_struct *napi, int budget)
 {
        struct ftgmac100 *priv = container_of(napi, struct ftgmac100, napi);
-       bool more, completed = true;
-       int rx = 0;
+       int work_done = 0;
+       bool more;
 
-       ftgmac100_tx_complete(priv);
+       /* Handle TX completions */
+       if (ftgmac100_tx_buf_cleanable(priv))
+               ftgmac100_tx_complete(priv);
 
+       /* Handle RX packets */
        do {
-               more = ftgmac100_rx_packet(priv, &rx);
-       } while (more && rx < budget);
-
-       if (more && rx == budget)
-               completed = false;
+               more = ftgmac100_rx_packet(priv, &work_done);
+       } while (more && work_done < budget);
 
 
        /* The interrupt is telling us to kick the MAC back to life
@@ -1004,11 +1036,13 @@ static int ftgmac100_poll(struct napi_struct *napi, int budget)
                          priv->base + FTGMAC100_OFFSET_IER);
        }
 
-       /* Keep NAPI going if we have still packets to reclaim */
-       if (priv->tx_pending)
-               return budget;
+       /* As long as we are waiting for transmit packets to be
+        * completed we keep NAPI going
+        */
+       if (ftgmac100_tx_buf_cleanable(priv))
+               work_done = budget;
 
-       if (completed) {
+       if (work_done < budget) {
                /* We are about to re-enable all interrupts. However
                 * the HW has been latching RX/TX packet interrupts while
                 * they were masked. So we clear them first, then we need
@@ -1016,7 +1050,8 @@ static int ftgmac100_poll(struct napi_struct *napi, int budget)
                 */
                iowrite32(FTGMAC100_INT_RXTX,
                          priv->base + FTGMAC100_OFFSET_ISR);
-               if (ftgmac100_check_rx(priv) || priv->tx_pending)
+               if (ftgmac100_check_rx(priv) ||
+                   ftgmac100_tx_buf_cleanable(priv))
                        return budget;
 
                /* deschedule NAPI */
@@ -1027,7 +1062,7 @@ static int ftgmac100_poll(struct napi_struct *napi, int budget)
                          priv->base + FTGMAC100_OFFSET_IER);
        }
 
-       return rx;
+       return work_done;
 }
 
 static int ftgmac100_init_all(struct ftgmac100 *priv, bool ignore_alloc_err)
@@ -1355,8 +1390,6 @@ static int ftgmac100_probe(struct platform_device *pdev)
        priv->dev = &pdev->dev;
        INIT_WORK(&priv->reset_task, ftgmac100_reset_task);
 
-       spin_lock_init(&priv->tx_lock);
-
        /* map io memory */
        priv->res = request_mem_region(res->start, resource_size(res),
                                       dev_name(&pdev->dev));