* [PATCH] rtw88: pci: enable MSI interrupt
@ 2019-07-30 11:50 yhchuang
2019-07-30 19:57 ` Brian Norris
0 siblings, 1 reply; 6+ messages in thread
From: yhchuang @ 2019-07-30 11:50 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, jano.vesely, briannorris
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.
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);
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];
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] rtw88: pci: enable MSI interrupt
2019-07-30 11:50 [PATCH] rtw88: pci: enable MSI interrupt yhchuang
@ 2019-07-30 19:57 ` Brian Norris
2019-08-01 9:21 ` Tony Chuang
0 siblings, 1 reply; 6+ messages in thread
From: Brian Norris @ 2019-07-30 19:57 UTC (permalink / raw)
To: yhchuang; +Cc: kvalo, linux-wireless, jano.vesely
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>
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH] rtw88: pci: enable MSI interrupt
2019-07-30 19:57 ` Brian Norris
@ 2019-08-01 9:21 ` Tony Chuang
2019-08-08 16:51 ` Brian Norris
0 siblings, 1 reply; 6+ messages in thread
From: Tony Chuang @ 2019-08-01 9:21 UTC (permalink / raw)
To: Brian Norris; +Cc: kvalo, linux-wireless, jano.vesely
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rtw88: pci: enable MSI interrupt
2019-08-01 9:21 ` Tony Chuang
@ 2019-08-08 16:51 ` Brian Norris
2019-08-20 15:21 ` Kai-Heng Feng
2019-08-22 2:26 ` Tony Chuang
0 siblings, 2 replies; 6+ messages in thread
From: Brian Norris @ 2019-08-08 16:51 UTC (permalink / raw)
To: Tony Chuang; +Cc: kvalo, linux-wireless, jano.vesely
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)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rtw88: pci: enable MSI interrupt
2019-08-08 16:51 ` Brian Norris
@ 2019-08-20 15:21 ` Kai-Heng Feng
2019-08-22 2:26 ` Tony Chuang
1 sibling, 0 replies; 6+ messages in thread
From: Kai-Heng Feng @ 2019-08-20 15:21 UTC (permalink / raw)
To: Brian Norris; +Cc: Tony Chuang, kvalo, linux-wireless, jano.vesely
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)
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] rtw88: pci: enable MSI interrupt
2019-08-08 16:51 ` Brian Norris
2019-08-20 15:21 ` Kai-Heng Feng
@ 2019-08-22 2:26 ` Tony Chuang
1 sibling, 0 replies; 6+ messages in thread
From: Tony Chuang @ 2019-08-22 2:26 UTC (permalink / raw)
To: Brian Norris; +Cc: kvalo, linux-wireless, jano.vesely
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-08-22 2:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-30 11:50 [PATCH] rtw88: pci: enable MSI interrupt yhchuang
2019-07-30 19:57 ` Brian Norris
2019-08-01 9:21 ` Tony Chuang
2019-08-08 16:51 ` Brian Norris
2019-08-20 15:21 ` Kai-Heng Feng
2019-08-22 2:26 ` Tony Chuang
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).