On Thu, Jul 27, 2023 at 9:07 AM Zhu Yanjun [off-list ref] wrote:

在 2023/7/26 21:15, Souradeep Chakrabarti 写道:

quoted

When unloading the MANA driver, mana_dealloc_queues() waits for the MANA
hardware to complete any inflight packets and set the pending send count
to zero. But if the hardware has failed, mana_dealloc_queues()
could wait forever.

Fix this by adding a timeout to the wait. Set the timeout to 120 seconds,
which is a somewhat arbitrary value that is more than long enough for
functional hardware to complete any sends.

Cc: stable@vger.kernel.org
Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")

Signed-off-by: Souradeep Chakrabarti <redacted>
---
V5 -> V6:
* Added pcie_flr to reset the pci after timeout.
* Fixed the position of changelog.
* Removed unused variable like cq.

V4 -> V5:
* Added fixes tag
* Changed the usleep_range from static to incremental value.
* Initialized timeout in the begining.

V3 -> V4:
* Removed the unnecessary braces from mana_dealloc_queues().

V2 -> V3:
* Removed the unnecessary braces from mana_dealloc_queues().

V1 -> V2:
* Added net branch
* Removed the typecasting to (struct mana_context*) of void pointer
* Repositioned timeout variable in mana_dealloc_queues()
* Repositioned vf_unload_timeout in mana_context struct, to utilise the
  6 bytes hole
---
  drivers/net/ethernet/microsoft/mana/mana_en.c | 38 +++++++++++++++++--
  1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index a499e460594b..ea039e2d4c4b 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c

@@ -8,6 +8,7 @@
  #include <linux/ethtool.h>
  #include <linux/filter.h>
  #include <linux/mm.h>
+#include <linux/pci.h>

  #include <net/checksum.h>
  #include <net/ip6_checksum.h>

@@ -2345,9 +2346,12 @@ int mana_attach(struct net_device *ndev)
  static int mana_dealloc_queues(struct net_device *ndev)
  {
      struct mana_port_context *apc = netdev_priv(ndev);
+     unsigned long timeout = jiffies + 120 * HZ;
      struct gdma_dev *gd = apc->ac->gdma_dev;
      struct mana_txq *txq;
+     struct sk_buff *skb;
      int i, err;
+     u32 tsleep;

      if (apc->port_is_up)
              return -EINVAL;

@@ -2363,15 +2367,41 @@ static int mana_dealloc_queues(struct net_device *ndev)
       * to false, but it doesn't matter since mana_start_xmit() drops any
       * new packets due to apc->port_is_up being false.
       *
-      * Drain all the in-flight TX packets
+      * Drain all the in-flight TX packets.
+      * A timeout of 120 seconds for all the queues is used.
+      * This will break the while loop when h/w is not responding.
+      * This value of 120 has been decided here considering max
+      * number of queues.
       */
+
      for (i = 0; i < apc->num_queues; i++) {
              txq = &apc->tx_qp[i].txq;
-
-             while (atomic_read(&txq->pending_sends) > 0)
-                     usleep_range(1000, 2000);
+             tsleep = 1000;
+             while (atomic_read(&txq->pending_sends) > 0 &&
+                    time_before(jiffies, timeout)) {
+                     usleep_range(tsleep, tsleep + 1000);
+                     tsleep <<= 1;
+             }
+             if (atomic_read(&txq->pending_sends)) {
+                     err  = pcie_flr(to_pci_dev(gd->gdma_context->dev));
+                     if (err) {
+                             netdev_err(ndev, "flr failed %d with %d pkts pending in txq %u\n",
+                                        err, atomic_read(&txq->pending_sends),
+                                        txq->gdma_txq_id);
+                     }
+                     break;
+             }
      }

+     for (i = 0; i < apc->num_queues; i++) {
+             txq = &apc->tx_qp[i].txq;
+             while (atomic_read(&txq->pending_sends)) {
+                     skb = skb_dequeue(&txq->pending_skbs);
+                     mana_unmap_skb(skb, apc);
+                     dev_consume_skb_any(skb);
+                     atomic_sub(1, &txq->pending_sends);
+             }

If I get this commit correctly, txq->pending_sends should be equal to
the length of txq->pending_skbs?

If yes, can we only handle the pending_skbs?

the above snippet can be changed to as below? So the performance is better?
"
                while ((skb = skb_dequeue(&txq->pending_skbs))) {
                        mana_unmap_skb(skb, apc);
                        dev_consume_skb_any(skb);
                }
                atomic_set(&txq->pending_sends, 0);
"

Zhu Yanjun

Yes, we can do that, thanks for pointing. Will take care of it in next version.

quoted

+     }
      /* We're 100% sure the queues can no longer be woken up, because
       * we're sure now mana_poll_tx_cq() can't be running.
       */