On Thu, Jul 27, 2023 at 9:07 AM Zhu Yanjun yanjun.zhu@linux.dev wrote:

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

...

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 schakrabarti@linux.microsoft.com

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.

...

• }
  /* 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.
   */