Re: [PATCH v3] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ

[Jian-Hong Pan <jian-hong@endlessm.com>]

Tony Chuang <yhchuang@realtek.com> 於 2019年8月21日 週三 下午4:16寫道：
>
> Hi,
>
> > From: Jian-Hong Pan [mailto:jian-hong@endlessm.com]
> >
> > There is a mass of jobs between spin lock and unlock in the hardware
> > IRQ which will occupy much time originally. To make system work more
> > efficiently, this patch moves the jobs to the soft IRQ (bottom half) to
> > reduce the time in hardware IRQ.
> >
> > Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com>
> > ---
> > v2:
> >  Change the spin_lock_irqsave/unlock_irqrestore to spin_lock/unlock in
> >  rtw_pci_interrupt_handler. Because the interrupts are already disabled
> >  in the hardware interrupt handler.
> >
> > v3:
> >  Extend the spin lock protecting area for the TX path in
> >  rtw_pci_interrupt_threadfn by Realtek's suggestion
> >
> >  drivers/net/wireless/realtek/rtw88/pci.c | 33 +++++++++++++++++++-----
> >  1 file changed, 27 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c
> > b/drivers/net/wireless/realtek/rtw88/pci.c
> > index 00ef229552d5..a8c17a01f318 100644
> > --- a/drivers/net/wireless/realtek/rtw88/pci.c
> > +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> > @@ -866,12 +866,29 @@ static irqreturn_t rtw_pci_interrupt_handler(int irq,
> > void *dev)
> >  {
> >       struct rtw_dev *rtwdev = dev;
> >       struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
> > -     u32 irq_status[4];
> >
> >       spin_lock(&rtwpci->irq_lock);
> >       if (!rtwpci->irq_enabled)
> >               goto out;
> >
> > +     /* disable RTW PCI interrupt to avoid more interrupts before the end of
> > +      * thread function
> > +      */
> > +     rtw_pci_disable_interrupt(rtwdev, rtwpci);
> > +out:
> > +     spin_unlock(&rtwpci->irq_lock);
> > +
> > +     return IRQ_WAKE_THREAD;
> > +}
> > +
> > +static irqreturn_t rtw_pci_interrupt_threadfn(int irq, void *dev)
> > +{
> > +     struct rtw_dev *rtwdev = dev;
> > +     struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
> > +     unsigned long flags;
> > +     u32 irq_status[4];
> > +
> > +     spin_lock_irqsave(&rtwpci->irq_lock, flags);
> >       rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status);
> >
> >       if (irq_status[0] & IMR_MGNTDOK)
> > @@ -891,8 +908,10 @@ static irqreturn_t rtw_pci_interrupt_handler(int irq,
> > void *dev)
> >       if (irq_status[0] & IMR_ROK)
> >               rtw_pci_rx_isr(rtwdev, rtwpci, RTW_RX_QUEUE_MPDU);
> >
> > -out:
> > -     spin_unlock(&rtwpci->irq_lock);
> > +     /* all of the jobs for this interrupt have been done */
> > +     if (rtw_flag_check(rtwdev, RTW_FLAG_RUNNING))
> > +             rtw_pci_enable_interrupt(rtwdev, rtwpci);
>
> I've applied this patch and tested it.
> But I failed to connect to AP, it seems that the
> scan_result is empty. And when I failed to connect
> to the AP, I found that the IMR is not enabled.
> I guess the check bypass the interrupt enable function.
>
> And I also found that *without MSI*, the driver is
> able to connect to the AP. Could you please verify
> this patch again with MSI interrupt enabled and
> send a v4?
>
> You can find my MSI patch on
> https://patchwork.kernel.org/patch/11081539/

I have just sent v4 patch.  Also tested the modified MSI patch like below:
The WiFi works fine on ASUS X512DK (including MSI enabled).

Jian-Hong Pan

diff --git a/drivers/net/wireless/realtek/rtw88/pci.c
b/drivers/net/wireless/realtek/rtw88/pci.c
index 955dd6c6fb57..d18f5aae1585 100644
--- a/drivers/net/wireless/realtek/rtw88/pci.c
+++ b/drivers/net/wireless/realtek/rtw88/pci.c
@@ -11,6 +11,10 @@
 #include "fw.h"
 #include "debug.h"

+static bool rtw_disable_msi;
+module_param_named(disable_msi, rtw_disable_msi, bool, 0644);
+MODULE_PARM_DESC(disable_msi, "Set Y to disable MSI interrupt support");
+
 static u32 rtw_pci_tx_queue_idx_addr[] = {
     [RTW_TX_QUEUE_BK]    = RTK_PCI_TXBD_IDX_BKQ,
     [RTW_TX_QUEUE_BE]    = RTK_PCI_TXBD_IDX_BEQ,
@@ -1116,6 +1120,48 @@ static struct rtw_hci_ops rtw_pci_ops = {
     .write_data_h2c = rtw_pci_write_data_h2c,
 };

+static int rtw_pci_request_irq(struct rtw_dev *rtwdev, struct pci_dev *pdev)
+{
+    struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+    int ret;
+
+    if (!rtw_disable_msi) {
+        ret = pci_enable_msi(pdev);
+        if (ret) {
+            rtw_warn(rtwdev, "failed to enable msi %d, using legacy irq\n",
+                 ret);
+        } else {
+            rtw_warn(rtwdev, "pci msi enabled\n");
+            rtwpci->msi_enabled = true;
+        }
+    }
+
+    ret = devm_request_threaded_irq(rtwdev->dev, pdev->irq,
+                    rtw_pci_interrupt_handler,
+                    rtw_pci_interrupt_threadfn,
+                    IRQF_SHARED, KBUILD_MODNAME, rtwdev);
+    if (ret) {
+        rtw_err(rtwdev, "failed to request irq %d\n", ret);
+        if (rtwpci->msi_enabled) {
+            pci_disable_msi(pdev);
+            rtwpci->msi_enabled = false;
+        }
+    }
+
+    return ret;
+}
+
+static void rtw_pci_free_irq(struct rtw_dev *rtwdev, struct pci_dev *pdev)
+{
+    struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+
+    devm_free_irq(rtwdev->dev, rtwpci->pdev->irq, rtwdev);
+    if (rtwpci->msi_enabled) {
+        pci_disable_msi(pdev);
+        rtwpci->msi_enabled = false;
+    }
+}
+
 static int rtw_pci_probe(struct pci_dev *pdev,
              const struct pci_device_id *id)
 {
@@ -1170,10 +1216,7 @@ static int rtw_pci_probe(struct pci_dev *pdev,
         goto err_destroy_pci;
     }

-    ret = devm_request_threaded_irq(rtwdev->dev, pdev->irq,
-                    rtw_pci_interrupt_handler,
-                    rtw_pci_interrupt_threadfn,
-                    IRQF_SHARED, KBUILD_MODNAME, rtwdev);
+    ret = rtw_pci_request_irq(rtwdev, pdev);
     if (ret) {
         ieee80211_unregister_hw(hw);
         goto err_destroy_pci;
@@ -1212,7 +1255,7 @@ static void rtw_pci_remove(struct pci_dev *pdev)
     rtw_pci_disable_interrupt(rtwdev, rtwpci);
     rtw_pci_destroy(rtwdev, pdev);
     rtw_pci_declaim(rtwdev, pdev);
-    devm_free_irq(rtwdev->dev, rtwpci->pdev->irq, rtwdev);
+    rtw_pci_free_irq(rtwdev, pdev);
     rtw_core_deinit(rtwdev);
     ieee80211_free_hw(hw);
 }
diff --git a/drivers/net/wireless/realtek/rtw88/pci.h
b/drivers/net/wireless/realtek/rtw88/pci.h
index 87824a4caba9..a8e369c5eaca 100644
--- a/drivers/net/wireless/realtek/rtw88/pci.h
+++ b/drivers/net/wireless/realtek/rtw88/pci.h
@@ -186,6 +186,7 @@ struct rtw_pci {
     spinlock_t irq_lock;
     u32 irq_mask[4];
     bool irq_enabled;
+    bool msi_enabled;

     u16 rx_tag;
     struct rtw_pci_tx_ring tx_rings[RTK_MAX_TX_QUEUE_NUM];


> > +     spin_unlock_irqrestore(&rtwpci->irq_lock, flags);
> >
> >       return IRQ_HANDLED;
> >  }
> > @@ -1152,8 +1171,10 @@ static int rtw_pci_probe(struct pci_dev *pdev,
> >               goto err_destroy_pci;
> >       }
> >
> > -     ret = request_irq(pdev->irq, &rtw_pci_interrupt_handler,
> > -                       IRQF_SHARED, KBUILD_MODNAME, rtwdev);
> > +     ret = devm_request_threaded_irq(rtwdev->dev, pdev->irq,
> > +                                     rtw_pci_interrupt_handler,
> > +                                     rtw_pci_interrupt_threadfn,
> > +                                     IRQF_SHARED, KBUILD_MODNAME, rtwdev);
> >       if (ret) {
> >               ieee80211_unregister_hw(hw);
> >               goto err_destroy_pci;
> > @@ -1192,7 +1213,7 @@ static void rtw_pci_remove(struct pci_dev *pdev)
> >       rtw_pci_disable_interrupt(rtwdev, rtwpci);
> >       rtw_pci_destroy(rtwdev, pdev);
> >       rtw_pci_declaim(rtwdev, pdev);
> > -     free_irq(rtwpci->pdev->irq, rtwdev);
> > +     devm_free_irq(rtwdev->dev, rtwpci->pdev->irq, rtwdev);
> >       rtw_core_deinit(rtwdev);
> >       ieee80211_free_hw(hw);
> >  }
> > --
>
>
> NACK
> Need to verify it with MSI https://patchwork.kernel.org/patch/11081539/
> And hope v4 could fix it.
>
> Yan-Hsuan
>