From: Yu-Yen Ting <[email protected]>
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ý <[email protected]>
Signed-off-by: Yu-Yen Ting <[email protected]>
Signed-off-by: Yan-Hsuan Chuang <[email protected]>
---
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
Hi,
On Tue, Jul 30, 2019 at 07:50:14PM +0800, [email protected] wrote:
> From: Yu-Yen Ting <[email protected]>
>
> 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? <[email protected]>
> Signed-off-by: Yu-Yen Ting <[email protected]>
> Signed-off-by: Yan-Hsuan Chuang <[email protected]>
> ---
> 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 <[email protected]>
> Subject: Re: [PATCH] rtw88: pci: enable MSI interrupt
>
> Hi,
>
> On Tue, Jul 30, 2019 at 07:50:14PM +0800, [email protected] wrote:
> > From: Yu-Yen Ting <[email protected]>
> >
> > 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? <[email protected]>
> > Signed-off-by: Yu-Yen Ting <[email protected]>
> > Signed-off-by: Yan-Hsuan Chuang <[email protected]>
> > ---
> > 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 <[email protected]>
>
Yan-Hsuan
On Thu, Aug 1, 2019 at 2:21 AM Tony Chuang <[email protected]> wrote:
> > Subject: Re: [PATCH] rtw88: pci: enable MSI interrupt
> > On Tue, Jul 30, 2019 at 07:50:14PM +0800, [email protected] 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 <[email protected]> wrote:
> On Thu, Aug 1, 2019 at 2:21 AM Tony Chuang <[email protected]> wrote:
>>> Subject: Re: [PATCH] rtw88: pci: enable MSI interrupt
>>> On Tue, Jul 30, 2019 at 07:50:14PM +0800, [email protected] 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 <[email protected]>
> wrote:
> > > Subject: Re: [PATCH] rtw88: pci: enable MSI interrupt
> > > On Tue, Jul 30, 2019 at 07:50:14PM +0800, [email protected]
> 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