Re: [PATCH V4 11/12] bnxt_en: Add TPH support in BNXT driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 9/16/24 19:55, Wei Huang wrote:


On 9/11/24 10:37 AM, Alejandro Lucero Palau wrote:

On 8/22/24 21:41, Wei Huang wrote:
From: Manoj Panicker <manoj.panicker2@xxxxxxx>

Implement TPH support in Broadcom BNXT device driver. The driver uses
TPH functions to retrieve and configure the device's Steering Tags when
its interrupt affinity is being changed.

Co-developed-by: Wei Huang <wei.huang2@xxxxxxx>
Signed-off-by: Wei Huang <wei.huang2@xxxxxxx>
Signed-off-by: Manoj Panicker <manoj.panicker2@xxxxxxx>
Reviewed-by: Ajit Khaparde <ajit.khaparde@xxxxxxxxxxxx>
Reviewed-by: Somnath Kotur <somnath.kotur@xxxxxxxxxxxx>
Reviewed-by: Andy Gospodarek <andrew.gospodarek@xxxxxxxxxxxx>
---
   drivers/net/ethernet/broadcom/bnxt/bnxt.c | 78 +++++++++++++++++++++++
   drivers/net/ethernet/broadcom/bnxt/bnxt.h |  4 ++
   2 files changed, 82 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index ffa74c26ee53..5903cd36b54d 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -55,6 +55,7 @@
   #include <net/page_pool/helpers.h>
   #include <linux/align.h>
   #include <net/netdev_queues.h>
+#include <linux/pci-tph.h>
      #include "bnxt_hsi.h"
   #include "bnxt.h"
@@ -10821,6 +10822,58 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
       return 0;
   }
   +static void __bnxt_irq_affinity_notify(struct irq_affinity_notify *notify,
+                       const cpumask_t *mask)
+{
+    struct bnxt_irq *irq;
+    u16 tag;
+
+    irq = container_of(notify, struct bnxt_irq, affinity_notify);
+    cpumask_copy(irq->cpu_mask, mask);
+
+    if (pcie_tph_get_cpu_st(irq->bp->pdev, TPH_MEM_TYPE_VM,
+                cpumask_first(irq->cpu_mask), &tag))


I understand just one cpu from the mask has to be used, but I wonder if
some check should be done for ensuring the mask is not mad.

This is control path and the related queue is going to be restarted, so
maybe a sanity check for ensuring all the cpus in the mask are from the
same CCX complex?

I don't think this is always true and we shouldn't warn when this happens. There is only one ST can be supported, so the driver need to make a good judgement on which ST to be used. But no matter what, ST is just a hint - it shouldn't cause any correctness issues in HW, even when it is not the optimal target CPU. So warning is unnecessary.


1) You can use a "mad" mask for avoiding a specific interrupt to disturb a specific execution is those cores not part of the mask. But I argue the ST hint should not be set then.


2) Someone, maybe an automatic script, could try to get the best performance possible, and a "mad" mask could preclude such outcome inadvertently.


I agree a warning could not be a good idea because 1, but I would say adding some way of traceability here could be interesting. A tracepoint or a new ST field for last hint set for that interrupt/queue.



That would be an iteration checking the tag is the same one for all of
them. If not, at least a warning stating the tag/CCX/cpu used.


+        return;
+
+    if (pcie_tph_set_st_entry(irq->bp->pdev, irq->msix_nr, tag))
+        return;
+
+    if (netif_running(irq->bp->dev)) {
+        rtnl_lock();
+        bnxt_close_nic(irq->bp, false, false);
+        bnxt_open_nic(irq->bp, false, false);
+        rtnl_unlock();
+    }
+}
+
+static void __bnxt_irq_affinity_release(struct kref __always_unused *ref)
+{
+}
+
+static void bnxt_release_irq_notifier(struct bnxt_irq *irq)
+{
+    irq_set_affinity_notifier(irq->vector, NULL);
+}
+
+static void bnxt_register_irq_notifier(struct bnxt *bp, struct bnxt_irq *irq)
+{
+    struct irq_affinity_notify *notify;
+
+    /* Nothing to do if TPH is not enabled */
+    if (!pcie_tph_enabled(bp->pdev))
+        return;
+
+    irq->bp = bp;
+
+    /* Register IRQ affinility notifier */
+    notify = &irq->affinity_notify;
+    notify->irq = irq->vector;
+    notify->notify = __bnxt_irq_affinity_notify;
+    notify->release = __bnxt_irq_affinity_release;
+
+    irq_set_affinity_notifier(irq->vector, notify);
+}
+
   static void bnxt_free_irq(struct bnxt *bp)
   {
       struct bnxt_irq *irq;
@@ -10843,11 +10896,17 @@ static void bnxt_free_irq(struct bnxt *bp)
                   free_cpumask_var(irq->cpu_mask);
                   irq->have_cpumask = 0;
               }
+
+            bnxt_release_irq_notifier(irq);
+
               free_irq(irq->vector, bp->bnapi[i]);
           }
              irq->requested = 0;
       }
+
+    /* Disable TPH support */
+    pcie_disable_tph(bp->pdev);
   }
      static int bnxt_request_irq(struct bnxt *bp)
@@ -10870,6 +10929,13 @@ static int bnxt_request_irq(struct bnxt *bp)
       if (!(bp->flags & BNXT_FLAG_USING_MSIX))
           flags = IRQF_SHARED;
   +    /* Enable TPH support as part of IRQ request */
+    if (pcie_tph_modes(bp->pdev) & PCI_TPH_CAP_INT_VEC) {
+        rc = pcie_enable_tph(bp->pdev, PCI_TPH_CAP_INT_VEC);
+        if (rc)
+            netdev_warn(bp->dev, "failed enabling TPH support\n");
+    }
+
       for (i = 0, j = 0; i < bp->cp_nr_rings; i++) {
           int map_idx = bnxt_cp_num_to_irq_num(bp, i);
           struct bnxt_irq *irq = &bp->irq_tbl[map_idx];
@@ -10893,8 +10959,10 @@ static int bnxt_request_irq(struct bnxt *bp)
              if (zalloc_cpumask_var(&irq->cpu_mask, GFP_KERNEL)) {
               int numa_node = dev_to_node(&bp->pdev->dev);
+            u16 tag;
                  irq->have_cpumask = 1;
+            irq->msix_nr = map_idx;
               cpumask_set_cpu(cpumask_local_spread(i, numa_node),
                       irq->cpu_mask);
               rc = irq_set_affinity_hint(irq->vector, irq->cpu_mask);
@@ -10904,6 +10972,16 @@ static int bnxt_request_irq(struct bnxt *bp)
                           irq->vector);
                   break;
               }
+
+            bnxt_register_irq_notifier(bp, irq);
+
+            /* Init ST table entry */
+            if (pcie_tph_get_cpu_st(irq->bp->pdev, TPH_MEM_TYPE_VM,
+                        cpumask_first(irq->cpu_mask),
+                        &tag))
+                break;
+
+            pcie_tph_set_st_entry(irq->bp->pdev, irq->msix_nr, tag);
           }
       }
       return rc;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 6bbdc718c3a7..ae1abcc1bddf 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -1224,6 +1224,10 @@ struct bnxt_irq {
       u8        have_cpumask:1;
       char        name[IFNAMSIZ + 2];
       cpumask_var_t    cpu_mask;
+
+    struct bnxt    *bp;
+    int        msix_nr;
+    struct irq_affinity_notify affinity_notify;
   };
      #define HWRM_RING_ALLOC_TX    0x1




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux