* [PATCH] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ @ 2019-08-16 6:31 Jian-Hong Pan 2019-08-16 7:54 ` Tony Chuang 0 siblings, 1 reply; 15+ messages in thread From: Jian-Hong Pan @ 2019-08-16 6:31 UTC (permalink / raw) To: Yan-Hsuan Chuang, Kalle Valo, David S . Miller Cc: linux-wireless, netdev, linux-kernel, linux, Jian-Hong Pan 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> --- drivers/net/wireless/realtek/rtw88/pci.c | 36 +++++++++++++++++++----- 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c index 00ef229552d5..355606b167c6 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]; + unsigned long flags; - spin_lock(&rtwpci->irq_lock); + spin_lock_irqsave(&rtwpci->irq_lock, flags); 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_irqrestore(&rtwpci->irq_lock, flags); + + 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]; + rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status); if (irq_status[0] & IMR_MGNTDOK) @@ -891,8 +908,11 @@ 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 */ + spin_lock_irqsave(&rtwpci->irq_lock, flags); + if (rtw_flag_check(rtwdev, RTW_FLAG_RUNNING)) + rtw_pci_enable_interrupt(rtwdev, rtwpci); + spin_unlock_irqrestore(&rtwpci->irq_lock, flags); return IRQ_HANDLED; } @@ -1152,8 +1172,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 +1214,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); } -- 2.20.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* RE: [PATCH] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ 2019-08-16 6:31 [PATCH] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ Jian-Hong Pan @ 2019-08-16 7:54 ` Tony Chuang 2019-08-16 8:06 ` Tony Chuang 0 siblings, 1 reply; 15+ messages in thread From: Tony Chuang @ 2019-08-16 7:54 UTC (permalink / raw) To: Jian-Hong Pan, Kalle Valo, David S . Miller Cc: linux-wireless, netdev, linux-kernel, linux > 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> > --- > drivers/net/wireless/realtek/rtw88/pci.c | 36 +++++++++++++++++++----- > 1 file changed, 29 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c > b/drivers/net/wireless/realtek/rtw88/pci.c > index 00ef229552d5..355606b167c6 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]; > + unsigned long flags; > > - spin_lock(&rtwpci->irq_lock); > + spin_lock_irqsave(&rtwpci->irq_lock, flags); > 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_irqrestore(&rtwpci->irq_lock, flags); > + > + 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]; > + > rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status); > > if (irq_status[0] & IMR_MGNTDOK) > @@ -891,8 +908,11 @@ 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 */ > + spin_lock_irqsave(&rtwpci->irq_lock, flags); Shouldn't we protect the ISRs above? This patch could actually reduce the time of IRQ. But I think I need to further test it with PCI MSI interrupt. https://patchwork.kernel.org/patch/11081539/ Maybe we could drop the "rtw_pci_[enable/disable]_interrupt" when MSI Is enabled with this patch. > + if (rtw_flag_check(rtwdev, RTW_FLAG_RUNNING)) > + rtw_pci_enable_interrupt(rtwdev, rtwpci); > + spin_unlock_irqrestore(&rtwpci->irq_lock, flags); > > return IRQ_HANDLED; > } > @@ -1152,8 +1172,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 +1214,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); > } > -- > 2.20.1 Yan-Hsuan ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ 2019-08-16 7:54 ` Tony Chuang @ 2019-08-16 8:06 ` Tony Chuang 2019-08-16 9:27 ` Jian-Hong Pan 0 siblings, 1 reply; 15+ messages in thread From: Tony Chuang @ 2019-08-16 8:06 UTC (permalink / raw) To: Tony Chuang, Jian-Hong Pan, Kalle Valo, David S . Miller Cc: linux-wireless, netdev, linux-kernel, linux Hi, A few more questions below > > 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> > > --- > > drivers/net/wireless/realtek/rtw88/pci.c | 36 +++++++++++++++++++----- > > 1 file changed, 29 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c > > b/drivers/net/wireless/realtek/rtw88/pci.c > > index 00ef229552d5..355606b167c6 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]; > > + unsigned long flags; > > > > - spin_lock(&rtwpci->irq_lock); > > + spin_lock_irqsave(&rtwpci->irq_lock, flags); I think you can use 'spin_lock()' here as it's in IRQ context? > > 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); Why do we need rtw_pci_disable_interrupt() here. Have you done any experiment and decided to add this. If you have can you share your results to me? > > +out: > > + spin_unlock_irqrestore(&rtwpci->irq_lock, flags); spin_unlock() > > + > > + 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]; > > + > > rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status); > > > > if (irq_status[0] & IMR_MGNTDOK) > > @@ -891,8 +908,11 @@ 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 */ > > + spin_lock_irqsave(&rtwpci->irq_lock, flags); > > Shouldn't we protect the ISRs above? > > This patch could actually reduce the time of IRQ. > But I think I need to further test it with PCI MSI interrupt. > https://patchwork.kernel.org/patch/11081539/ > > Maybe we could drop the "rtw_pci_[enable/disable]_interrupt" when MSI > Is enabled with this patch. > > > + if (rtw_flag_check(rtwdev, RTW_FLAG_RUNNING)) > > + rtw_pci_enable_interrupt(rtwdev, rtwpci); > > + spin_unlock_irqrestore(&rtwpci->irq_lock, flags); > > > > return IRQ_HANDLED; > > } > > @@ -1152,8 +1172,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 +1214,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); > > } > > -- > > 2.20.1 > > Yan-Hsuan > Thanks Yan-Hsuan ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ 2019-08-16 8:06 ` Tony Chuang @ 2019-08-16 9:27 ` Jian-Hong Pan 2019-08-16 10:09 ` [PATCH v2] " Jian-Hong Pan 0 siblings, 1 reply; 15+ messages in thread From: Jian-Hong Pan @ 2019-08-16 9:27 UTC (permalink / raw) To: Tony Chuang Cc: Kalle Valo, David S . Miller, linux-wireless, netdev, linux-kernel, linux Tony Chuang <yhchuang@realtek.com> 於 2019年8月16日 週五 下午4:07寫道: > > Hi, > > A few more questions below > > > > 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> > > > --- > > > drivers/net/wireless/realtek/rtw88/pci.c | 36 +++++++++++++++++++----- > > > 1 file changed, 29 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c > > > b/drivers/net/wireless/realtek/rtw88/pci.c > > > index 00ef229552d5..355606b167c6 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]; > > > + unsigned long flags; > > > > > > - spin_lock(&rtwpci->irq_lock); > > > + spin_lock_irqsave(&rtwpci->irq_lock, flags); > > I think you can use 'spin_lock()' here as it's in IRQ context? Ah! You are right! The interrupts are already disabled in the interrupt handler. So, there is no need to disable more once. I can tweak it. > > > 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); > > > Why do we need rtw_pci_disable_interrupt() here. > Have you done any experiment and decided to add this. > If you have can you share your results to me? I got this idea "Avoid back to back interrupt" from Intel WiFi's driver. https://elixir.bootlin.com/linux/v5.3-rc4/source/drivers/net/wireless/intel/iwlwifi/pcie/rx.c#L2071 So, I disable rtw_pci interrupt here in first half IRQ. (Re-enable in bottom half) > > > > +out: > > > + spin_unlock_irqrestore(&rtwpci->irq_lock, flags); > > spin_unlock() > > > > + > > > + 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]; > > > + > > > rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status); > > > > > > if (irq_status[0] & IMR_MGNTDOK) > > > @@ -891,8 +908,11 @@ 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 */ > > > + spin_lock_irqsave(&rtwpci->irq_lock, flags); > > > > Shouldn't we protect the ISRs above? > > > > This patch could actually reduce the time of IRQ. > > But I think I need to further test it with PCI MSI interrupt. > > https://patchwork.kernel.org/patch/11081539/ > > > > Maybe we could drop the "rtw_pci_[enable/disable]_interrupt" when MSI > > Is enabled with this patch. > > > > > + if (rtw_flag_check(rtwdev, RTW_FLAG_RUNNING)) > > > + rtw_pci_enable_interrupt(rtwdev, rtwpci); Then, re-enable rtw_pci interrupt here in bottom half of the IRQ. Here is the place where Intel WiFi re-enable interrupts. https://elixir.bootlin.com/linux/v5.3-rc4/source/drivers/net/wireless/intel/iwlwifi/pcie/rx.c#L1959 Now, we can go back to the question "Shouldn't we protect the ISRs above?" 1. What does the lock: rtwpci->irq_lock protect for? According to the code, seems only rtw_pci interrupt's state which is enabled or not. 2. How about the ISRs you mentioned? This part will only be executed if there is a fresh rtw_pci interrupt. The first half already disabled rtw_pci interrupt, so there is no more fresh rtw_pci interrupt until rtw_pci interrupt is enabled again. Therefor, the rtwpci->irq_lock only wraps the rtw_pci interrupt enablement. If there is any more entry that I missed and will interfere, please let me know. Thank you Jian-Hong Pan ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ 2019-08-16 9:27 ` Jian-Hong Pan @ 2019-08-16 10:09 ` Jian-Hong Pan 2019-08-16 10:44 ` Tony Chuang 0 siblings, 1 reply; 15+ messages in thread From: Jian-Hong Pan @ 2019-08-16 10:09 UTC (permalink / raw) To: Yan-Hsuan Chuang, Kalle Valo, David S . Miller Cc: linux-wireless, netdev, linux-kernel, linux, Jian-Hong Pan 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. 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..0740140d7e46 100644 --- a/drivers/net/wireless/realtek/rtw88/pci.c +++ b/drivers/net/wireless/realtek/rtw88/pci.c @@ -866,12 +866,28 @@ 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]; + rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status); if (irq_status[0] & IMR_MGNTDOK) @@ -891,8 +907,11 @@ 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 */ + spin_lock_irqsave(&rtwpci->irq_lock, flags); + if (rtw_flag_check(rtwdev, RTW_FLAG_RUNNING)) + rtw_pci_enable_interrupt(rtwdev, rtwpci); + 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); } -- 2.20.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* RE: [PATCH v2] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ 2019-08-16 10:09 ` [PATCH v2] " Jian-Hong Pan @ 2019-08-16 10:44 ` Tony Chuang 2019-08-19 3:26 ` Jian-Hong Pan 0 siblings, 1 reply; 15+ messages in thread From: Tony Chuang @ 2019-08-16 10:44 UTC (permalink / raw) To: Jian-Hong Pan, Kalle Valo, David S . Miller Cc: linux-wireless, netdev, linux-kernel, linux > From: Jian-Hong Pan > > 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. > > 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..0740140d7e46 100644 > --- a/drivers/net/wireless/realtek/rtw88/pci.c > +++ b/drivers/net/wireless/realtek/rtw88/pci.c > @@ -866,12 +866,28 @@ 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); So basically it's to prevent back-to-back interrupts. Nothing wrong about it, I just wondering why we don't like back-to-back interrupts. Does it means that those interrupts fired between irq_handler and threadfin would increase much more time to consume them. > +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]; > + > rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status); > > if (irq_status[0] & IMR_MGNTDOK) > @@ -891,8 +907,11 @@ 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 */ > + spin_lock_irqsave(&rtwpci->irq_lock, flags); I suggest to protect the ISRs. Because next patches will require to check if the TX DMA path is empty. This means I will also add this rtwpci->irq_lock to the TX path, and check if the skb_queue does not have any pending SKBs not DMAed successfully. > + if (rtw_flag_check(rtwdev, RTW_FLAG_RUNNING)) Why check the flag here? Is there any racing or something? Otherwise it looks to break the symmetry. > + rtw_pci_enable_interrupt(rtwdev, rtwpci); > + 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); > } > -- > 2.20.1 Yan-Hsuan ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ 2019-08-16 10:44 ` Tony Chuang @ 2019-08-19 3:26 ` Jian-Hong Pan 2019-08-20 4:59 ` [PATCH v3] " Jian-Hong Pan 0 siblings, 1 reply; 15+ messages in thread From: Jian-Hong Pan @ 2019-08-19 3:26 UTC (permalink / raw) To: Tony Chuang Cc: Kalle Valo, David S . Miller, linux-wireless, netdev, linux-kernel, linux Tony Chuang <yhchuang@realtek.com> 於 2019年8月16日 週五 下午6:44寫道: > > > From: Jian-Hong Pan > > > > 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. > > > > 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..0740140d7e46 100644 > > --- a/drivers/net/wireless/realtek/rtw88/pci.c > > +++ b/drivers/net/wireless/realtek/rtw88/pci.c > > @@ -866,12 +866,28 @@ 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); > > So basically it's to prevent back-to-back interrupts. > > Nothing wrong about it, I just wondering why we don't like > back-to-back interrupts. Does it means that those interrupts > fired between irq_handler and threadfin would increase > much more time to consume them. 1. It is one of the reasons. Besides, the hardware interrupt has higher priority than soft IRQ. If next hardware interrupt is coming faster then the soft IRQ (BH), the software IRQ may always become pended. 2. Keep the data's state until the interrupt is fully dealt. 3. The process of request_threaded_irq is documented: https://www.kernel.org/doc/htmldocs/kernel-api/API-request-threaded-irq.html > > +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]; > > + > > rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status); > > > > if (irq_status[0] & IMR_MGNTDOK) > > @@ -891,8 +907,11 @@ 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 */ > > + spin_lock_irqsave(&rtwpci->irq_lock, flags); > > I suggest to protect the ISRs. Because next patches will require > to check if the TX DMA path is empty. This means I will also add > this rtwpci->irq_lock to the TX path, and check if the skb_queue > does not have any pending SKBs not DMAed successfully. Ah ... Okay for the TX path > > + if (rtw_flag_check(rtwdev, RTW_FLAG_RUNNING)) > > Why check the flag here? Is there any racing or something? > Otherwise it looks to break the symmetry. According to backtrace, I notice rtw_pci_stop will be invoked in rtw_power_off, rtw_core_stop which clears the running state. rtw_core_stop is invoked by rtw_enter_ips due to IEEE80211_CONF_IDLE. If the stop command comes between the hardware interrupt and soft IRQ (BH) and there is no start command again, I think user wants the WiFi stop instead of becoming start again. Jian-Hong Pan ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ 2019-08-19 3:26 ` Jian-Hong Pan @ 2019-08-20 4:59 ` Jian-Hong Pan 2019-08-21 8:16 ` Tony Chuang 0 siblings, 1 reply; 15+ messages in thread From: Jian-Hong Pan @ 2019-08-20 4:59 UTC (permalink / raw) To: Yan-Hsuan Chuang, Kalle Valo, David S . Miller Cc: linux-wireless, netdev, linux-kernel, linux, Jian-Hong Pan 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); + 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); } -- 2.20.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* RE: [PATCH v3] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ 2019-08-20 4:59 ` [PATCH v3] " Jian-Hong Pan @ 2019-08-21 8:16 ` Tony Chuang 2019-08-26 7:08 ` [PATCH v4] " Jian-Hong Pan 2019-08-26 7:21 ` [PATCH v3] " Jian-Hong Pan 0 siblings, 2 replies; 15+ messages in thread From: Tony Chuang @ 2019-08-21 8:16 UTC (permalink / raw) To: Jian-Hong Pan, Kalle Valo, David S . Miller Cc: linux-wireless, netdev, linux-kernel, linux 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/ > + 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ 2019-08-21 8:16 ` Tony Chuang @ 2019-08-26 7:08 ` Jian-Hong Pan 2019-08-27 9:20 ` Tony Chuang 2019-08-26 7:21 ` [PATCH v3] " Jian-Hong Pan 1 sibling, 1 reply; 15+ messages in thread From: Jian-Hong Pan @ 2019-08-26 7:08 UTC (permalink / raw) To: Yan-Hsuan Chuang, Kalle Valo, David S . Miller Cc: linux-wireless, netdev, linux-kernel, linux, Jian-Hong Pan 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 v4: Remove the WiFi running check in rtw_pci_interrupt_threadfn to avoid AP connection failed by Realtek's suggestion. drivers/net/wireless/realtek/rtw88/pci.c | 32 +++++++++++++++++++----- 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c index 00ef229552d5..955dd6c6fb57 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,9 @@ 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 */ + rtw_pci_enable_interrupt(rtwdev, rtwpci); + spin_unlock_irqrestore(&rtwpci->irq_lock, flags); return IRQ_HANDLED; } @@ -1152,8 +1170,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 +1212,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); } -- 2.20.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* RE: [PATCH v4] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ 2019-08-26 7:08 ` [PATCH v4] " Jian-Hong Pan @ 2019-08-27 9:20 ` Tony Chuang 2019-09-02 12:18 ` Kalle Valo 0 siblings, 1 reply; 15+ messages in thread From: Tony Chuang @ 2019-08-27 9:20 UTC (permalink / raw) To: Jian-Hong Pan, Kalle Valo, David S . Miller Cc: linux-wireless, netdev, linux-kernel, linux > From: Jian-Hong Pan > Subject: [PATCH v4] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ > > 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 > > v4: > Remove the WiFi running check in rtw_pci_interrupt_threadfn to avoid AP > connection failed by Realtek's suggestion. > > drivers/net/wireless/realtek/rtw88/pci.c | 32 +++++++++++++++++++----- > 1 file changed, 26 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c > b/drivers/net/wireless/realtek/rtw88/pci.c > index 00ef229552d5..955dd6c6fb57 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,9 @@ 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 */ > + rtw_pci_enable_interrupt(rtwdev, rtwpci); > + spin_unlock_irqrestore(&rtwpci->irq_lock, flags); > > return IRQ_HANDLED; > } > @@ -1152,8 +1170,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 +1212,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); > } > -- > 2.20.1 Now it works fine with MSI interrupt enabled. But this patch is conflicting with MSI interrupt patch. Is there a better way we can make Kalle apply them more smoothly? I can rebase them and submit both if you're OK. Yan-Hsuan ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ 2019-08-27 9:20 ` Tony Chuang @ 2019-09-02 12:18 ` Kalle Valo 2019-09-03 2:45 ` Jian-Hong Pan 0 siblings, 1 reply; 15+ messages in thread From: Kalle Valo @ 2019-09-02 12:18 UTC (permalink / raw) To: Tony Chuang Cc: Jian-Hong Pan, David S . Miller, linux-wireless, netdev, linux-kernel, linux Tony Chuang <yhchuang@realtek.com> writes: >> From: Jian-Hong Pan >> Subject: [PATCH v4] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ >> >> 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> > > Now it works fine with MSI interrupt enabled. > > But this patch is conflicting with MSI interrupt patch. > Is there a better way we can make Kalle apply them more smoothly? > I can rebase them and submit both if you're OK. Yeah, submitting all the MSI patches in the same patchset is the easiest approach. That way they apply cleanly to wireless-drivers-next. -- https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ 2019-09-02 12:18 ` Kalle Valo @ 2019-09-03 2:45 ` Jian-Hong Pan 2019-09-03 9:18 ` Tony Chuang 0 siblings, 1 reply; 15+ messages in thread From: Jian-Hong Pan @ 2019-09-03 2:45 UTC (permalink / raw) To: Kalle Valo Cc: Tony Chuang, David S . Miller, linux-wireless, netdev, linux-kernel, linux Kalle Valo <kvalo@codeaurora.org> 於 2019年9月2日 週一 下午8:18寫道: > > Tony Chuang <yhchuang@realtek.com> writes: > > >> From: Jian-Hong Pan > >> Subject: [PATCH v4] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ > >> > >> 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> > > > > Now it works fine with MSI interrupt enabled. > > > > But this patch is conflicting with MSI interrupt patch. > > Is there a better way we can make Kalle apply them more smoothly? > > I can rebase them and submit both if you're OK. The rebase work is appreciated. Thank you, Jian-Hong Pan > Yeah, submitting all the MSI patches in the same patchset is the easiest > approach. That way they apply cleanly to wireless-drivers-next. > > -- > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH v4] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ 2019-09-03 2:45 ` Jian-Hong Pan @ 2019-09-03 9:18 ` Tony Chuang 0 siblings, 0 replies; 15+ messages in thread From: Tony Chuang @ 2019-09-03 9:18 UTC (permalink / raw) To: Jian-Hong Pan, Kalle Valo Cc: David S . Miller, linux-wireless, netdev, linux-kernel, linux > From: Jian-Hong Pan [mailto:jian-hong@endlessm.com] > > > > > Tony Chuang <yhchuang@realtek.com> writes: > > > > >> From: Jian-Hong Pan > > >> Subject: [PATCH v4] rtw88: pci: Move a mass of jobs in hw IRQ to soft > IRQ > > >> > > >> 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> > > > > > > Now it works fine with MSI interrupt enabled. > > > > > > But this patch is conflicting with MSI interrupt patch. > > > Is there a better way we can make Kalle apply them more smoothly? > > > I can rebase them and submit both if you're OK. > > The rebase work is appreciated. > Rebased and sent. Please check it and see if I've done anything wrong :) https://patchwork.kernel.org/cover/11127453/ Thanks, Yan-Hsuan ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ 2019-08-21 8:16 ` Tony Chuang 2019-08-26 7:08 ` [PATCH v4] " Jian-Hong Pan @ 2019-08-26 7:21 ` Jian-Hong Pan 1 sibling, 0 replies; 15+ messages in thread From: Jian-Hong Pan @ 2019-08-26 7:21 UTC (permalink / raw) To: Tony Chuang Cc: Kalle Valo, David S . Miller, linux-wireless, netdev, linux-kernel, linux 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 > ^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-09-03 9:19 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-16 6:31 [PATCH] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ Jian-Hong Pan 2019-08-16 7:54 ` Tony Chuang 2019-08-16 8:06 ` Tony Chuang 2019-08-16 9:27 ` Jian-Hong Pan 2019-08-16 10:09 ` [PATCH v2] " Jian-Hong Pan 2019-08-16 10:44 ` Tony Chuang 2019-08-19 3:26 ` Jian-Hong Pan 2019-08-20 4:59 ` [PATCH v3] " Jian-Hong Pan 2019-08-21 8:16 ` Tony Chuang 2019-08-26 7:08 ` [PATCH v4] " Jian-Hong Pan 2019-08-27 9:20 ` Tony Chuang 2019-09-02 12:18 ` Kalle Valo 2019-09-03 2:45 ` Jian-Hong Pan 2019-09-03 9:18 ` Tony Chuang 2019-08-26 7:21 ` [PATCH v3] " Jian-Hong Pan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).