| Message ID | 1564487414-9615-1-git-send-email-yhchuang@realtek.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | rtw88: pci: enable MSI interrupt | expand |
Hi, On Tue, Jul 30, 2019 at 07:50:14PM +0800, yhchuang@realtek.com wrote: > From: Yu-Yen Ting <steventing@realtek.com> > > MSI interrupt should be enabled on certain platform. > > Add a module parameter disable_msi to disable MSI interrupt, > driver will then use legacy interrupt instead. > And the interrupt mode is not able to change at run-time, so > the module parameter is read only. Well, if we unbind/rebind the device, probe() will pick up the new value. e.g.: echo '0000:01:00.0' > /sys/bus/pci/drivers/rtw_pci/unbind echo '0000:01:00.0' > /sys/bus/pci/drivers/rtw_pci/bind So is it really necessary to mark read-only? I think there's a general understanding that module parameters are not always "immediately effective." > Tested-by: Ján Veselý <jano.vesely@gmail.com> > Signed-off-by: Yu-Yen Ting <steventing@realtek.com> > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com> > --- > drivers/net/wireless/realtek/rtw88/pci.c | 51 ++++++++++++++++++++++++++++++-- > drivers/net/wireless/realtek/rtw88/pci.h | 1 + > 2 files changed, 49 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c > index 23dd06a..25410f6 100644 > --- a/drivers/net/wireless/realtek/rtw88/pci.c > +++ b/drivers/net/wireless/realtek/rtw88/pci.c > @@ -10,6 +10,10 @@ > #include "rx.h" > #include "debug.h" > > +static bool rtw_disable_msi; > +module_param_named(disable_msi, rtw_disable_msi, bool, 0444); > +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, > @@ -874,6 +878,7 @@ static irqreturn_t rtw_pci_interrupt_handler(int irq, void *dev) > if (!rtwpci->irq_enabled) > goto out; > > + rtw_pci_disable_interrupt(rtwdev, rtwpci); Why exactly do you have to mask interrupts during the ISR? Is there a race in rtw_pci_irq_recognized() or something? > rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status); > > if (irq_status[0] & IMR_MGNTDOK) ... > @@ -1103,6 +1110,45 @@ 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, using legacy irq\n"); > + } else { > + rtw_warn(rtwdev, "pci msi enabled\n"); > + rtwpci->msi_enabled = true; > + } > + } > + > + ret = request_irq(pdev->irq, &rtw_pci_interrupt_handler, IRQF_SHARED, > + KBUILD_MODNAME, rtwdev); > + if (ret) { > + rtw_err(rtwdev, "failed to request irq\n"); Print out 'ret' here? > + if (rtwpci->msi_enabled) { > + pci_disable_msi(pdev); > + rtwpci->msi_enabled = false; > + } > + } > + > + return ret; > +} Otherwise, looks fine: Reviewed-by: Brian Norris <briannorris@chromium.org>
> Subject: Re: [PATCH] rtw88: pci: enable MSI interrupt > > Hi, > > On Tue, Jul 30, 2019 at 07:50:14PM +0800, yhchuang@realtek.com wrote: > > From: Yu-Yen Ting <steventing@realtek.com> > > > > MSI interrupt should be enabled on certain platform. > > > > Add a module parameter disable_msi to disable MSI interrupt, > > driver will then use legacy interrupt instead. > > And the interrupt mode is not able to change at run-time, so > > the module parameter is read only. > > Well, if we unbind/rebind the device, probe() will pick up the new > value. e.g.: > > echo '0000:01:00.0' > /sys/bus/pci/drivers/rtw_pci/unbind > echo '0000:01:00.0' > /sys/bus/pci/drivers/rtw_pci/bind > > So is it really necessary to mark read-only? I think there's a general > understanding that module parameters are not always "immediately > effective." If there's a general understanding of not always effective immediately, I think I can change the file mode to 0644. > > > Tested-by: Ján Veselý <jano.vesely@gmail.com> > > Signed-off-by: Yu-Yen Ting <steventing@realtek.com> > > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com> > > --- > > drivers/net/wireless/realtek/rtw88/pci.c | 51 > ++++++++++++++++++++++++++++++-- > > drivers/net/wireless/realtek/rtw88/pci.h | 1 + > > 2 files changed, 49 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c > b/drivers/net/wireless/realtek/rtw88/pci.c > > index 23dd06a..25410f6 100644 > > --- a/drivers/net/wireless/realtek/rtw88/pci.c > > +++ b/drivers/net/wireless/realtek/rtw88/pci.c > > @@ -874,6 +878,7 @@ static irqreturn_t rtw_pci_interrupt_handler(int irq, > void *dev) > > if (!rtwpci->irq_enabled) > > goto out; > > > > + rtw_pci_disable_interrupt(rtwdev, rtwpci); > > Why exactly do you have to mask interrupts during the ISR? Is there a > race in rtw_pci_irq_recognized() or something? I think there is a race between SW and HW, if we do not stop the IRQ first, write 1 clear will make the interrupt to be lost. > > > rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status); > > > > if (irq_status[0] & IMR_MGNTDOK) > > ... > > > Otherwise, looks fine: > > Reviewed-by: Brian Norris <briannorris@chromium.org> > Yan-Hsuan
On Thu, Aug 1, 2019 at 2:21 AM Tony Chuang <yhchuang@realtek.com> wrote: > > Subject: Re: [PATCH] rtw88: pci: enable MSI interrupt > > On Tue, Jul 30, 2019 at 07:50:14PM +0800, yhchuang@realtek.com wrote: > > > --- a/drivers/net/wireless/realtek/rtw88/pci.c > > > +++ b/drivers/net/wireless/realtek/rtw88/pci.c > > > @@ -874,6 +878,7 @@ static irqreturn_t rtw_pci_interrupt_handler(int irq, > > void *dev) > > > if (!rtwpci->irq_enabled) > > > goto out; > > > > > > + rtw_pci_disable_interrupt(rtwdev, rtwpci); > > > > Why exactly do you have to mask interrupts during the ISR? Is there a > > race in rtw_pci_irq_recognized() or something? > > > I think there is a race between SW and HW, if we do not stop the > IRQ first, write 1 clear will make the interrupt to be lost. This doesn't need to slow down this patch (I think v2 is fine), but I still don't quite understand. Before this addition, the sequence is: (a) read out your IRQ status (b) ack the un-masked IRQs you see (c) operate on those IRQs Even if a new IRQ comes in the middle of (b), shouldn't it be sufficient to move on to (c), where you're still prepared to handle that IRQ? Or if the IRQ comes after (b), you won't ACK it, and you should immediately get a new IRQ after you return? I guess that's assuming that these registers are Write 1 to Clear. But if so, that means rtw_pci_irq_recognized() is effectively atomic, no? Also, somewhat unrelated: but why do you unmask HIMR1, when you're not actually handling any of its IRQ bits? Brian > > > > > rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status); > > > > > > if (irq_status[0] & IMR_MGNTDOK)
at 00:51, Brian Norris <briannorris@chromium.org> wrote: > On Thu, Aug 1, 2019 at 2:21 AM Tony Chuang <yhchuang@realtek.com> wrote: >>> Subject: Re: [PATCH] rtw88: pci: enable MSI interrupt >>> On Tue, Jul 30, 2019 at 07:50:14PM +0800, yhchuang@realtek.com wrote: >>>> --- a/drivers/net/wireless/realtek/rtw88/pci.c >>>> +++ b/drivers/net/wireless/realtek/rtw88/pci.c >>>> @@ -874,6 +878,7 @@ static irqreturn_t rtw_pci_interrupt_handler(int >>>> irq, >>> void *dev) >>>> if (!rtwpci->irq_enabled) >>>> goto out; >>>> >>>> + rtw_pci_disable_interrupt(rtwdev, rtwpci); >>> >>> Why exactly do you have to mask interrupts during the ISR? Is there a >>> race in rtw_pci_irq_recognized() or something? >> >> >> I think there is a race between SW and HW, if we do not stop the >> IRQ first, write 1 clear will make the interrupt to be lost. > > This doesn't need to slow down this patch (I think v2 is fine), but I > still don't quite understand. Before this addition, the sequence is: > (a) read out your IRQ status > (b) ack the un-masked IRQs you see > (c) operate on those IRQs > > Even if a new IRQ comes in the middle of (b), shouldn't it be > sufficient to move on to (c), where you're still prepared to handle > that IRQ? > > Or if the IRQ comes after (b), you won't ACK it, and you should > immediately get a new IRQ after you return? > > I guess that's assuming that these registers are Write 1 to Clear. But > if so, that means rtw_pci_irq_recognized() is effectively atomic, no? > > Also, somewhat unrelated: but why do you unmask HIMR1, when you're not > actually handling any of its IRQ bits? According to user feedback [1], this patch makes rtw88 work. I’ll make it merged into Ubuntu’s kernel for now, hopefully this will be in upstream version soon. [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1838133/comments/48 Kai-Heng > > Brian > >>>> rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status); >>>> >>>> if (irq_status[0] & IMR_MGNTDOK)
> On Behalf Of Brian Norris > On Thu, Aug 1, 2019 at 2:21 AM Tony Chuang <yhchuang@realtek.com> > wrote: > > > Subject: Re: [PATCH] rtw88: pci: enable MSI interrupt > > > On Tue, Jul 30, 2019 at 07:50:14PM +0800, yhchuang@realtek.com > wrote: > > > > --- a/drivers/net/wireless/realtek/rtw88/pci.c > > > > +++ b/drivers/net/wireless/realtek/rtw88/pci.c > > > > @@ -874,6 +878,7 @@ static irqreturn_t rtw_pci_interrupt_handler(int > irq, > > > void *dev) > > > > if (!rtwpci->irq_enabled) > > > > goto out; > > > > > > > > + rtw_pci_disable_interrupt(rtwdev, rtwpci); > > > > > > Why exactly do you have to mask interrupts during the ISR? Is there a > > > race in rtw_pci_irq_recognized() or something? > > > > > > I think there is a race between SW and HW, if we do not stop the > > IRQ first, write 1 clear will make the interrupt to be lost. > > This doesn't need to slow down this patch (I think v2 is fine), but I > still don't quite understand. Before this addition, the sequence is: > (a) read out your IRQ status > (b) ack the un-masked IRQs you see > (c) operate on those IRQs > > Even if a new IRQ comes in the middle of (b), shouldn't it be > sufficient to move on to (c), where you're still prepared to handle > that IRQ? > > Or if the IRQ comes after (b), you won't ACK it, and you should > immediately get a new IRQ after you return? I think it's because that MSI interrupts are edge-triggered. If the interrupt comes when IRQ is being processed, the interrupt won't be received. If the interrupt is not received, the interrupt won't be Write-1-Cleared, and won't be fired again. So driver should disable the interrupt until the ISRs are done. > > I guess that's assuming that these registers are Write 1 to Clear. But > if so, that means rtw_pci_irq_recognized() is effectively atomic, no? > > Also, somewhat unrelated: but why do you unmask HIMR1, when you're not > actually handling any of its IRQ bits? We could use HIMR1, just not handling any of them now :) > > Brian > Yan-Hsuan
diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c index 23dd06a..25410f6 100644 --- a/drivers/net/wireless/realtek/rtw88/pci.c +++ b/drivers/net/wireless/realtek/rtw88/pci.c @@ -10,6 +10,10 @@ #include "rx.h" #include "debug.h" +static bool rtw_disable_msi; +module_param_named(disable_msi, rtw_disable_msi, bool, 0444); +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, @@ -874,6 +878,7 @@ static irqreturn_t rtw_pci_interrupt_handler(int irq, void *dev) if (!rtwpci->irq_enabled) goto out; + rtw_pci_disable_interrupt(rtwdev, rtwpci); rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status); if (irq_status[0] & IMR_MGNTDOK) @@ -893,6 +898,8 @@ 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); + rtw_pci_enable_interrupt(rtwdev, rtwpci); + out: spin_unlock(&rtwpci->irq_lock); @@ -1103,6 +1110,45 @@ 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, using legacy irq\n"); + } else { + rtw_warn(rtwdev, "pci msi enabled\n"); + rtwpci->msi_enabled = true; + } + } + + ret = request_irq(pdev->irq, &rtw_pci_interrupt_handler, IRQF_SHARED, + KBUILD_MODNAME, rtwdev); + if (ret) { + rtw_err(rtwdev, "failed to request irq\n"); + 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; + + free_irq(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) { @@ -1157,8 +1203,7 @@ 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 = rtw_pci_request_irq(rtwdev, pdev); if (ret) { ieee80211_unregister_hw(hw); goto err_destroy_pci; @@ -1197,7 +1242,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); + 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 87824a4..a8e369c 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];