[PATCH v2] rtw88: pci: enable MSI interrupt

[PATCH v2] rtw88: pci: enable MSI interrupt

[yhchuang]

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.

One could rebind the PCI device, probe() will pick up the
new value of the module parameter. Such as:

    echo '0000:01:00.0' > /sys/bus/pci/drivers/rtw_pci/unbind
    echo '0000:01:00.0' > /sys/bus/pci/drivers/rtw_pci/bind

Tested-by: Ján Veselý <jano.vesely@gmail.com>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Yu-Yen Ting <steventing@realtek.com>
Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
---

v1 -> v2
    * Module parameter file mode change from 0444 to 0644
      This allows to change interrupt mode at run-time,
      as user rebind the PCI device
    * Print out returned value for pci_enable_msi(), as the
      value is not going to be returned to upper stacks
    * Print out request_irq() return value if it failed

 drivers/net/wireless/realtek/rtw88/pci.c | 52 ++++++++++++++++++++++++++++++--
 drivers/net/wireless/realtek/rtw88/pci.h |  1 +
 2 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
index 00ef229..c8c7951 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,
@@ -872,6 +876,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)
@@ -891,6 +896,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);
 
@@ -1098,6 +1105,46 @@ 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 = request_irq(pdev->irq, &rtw_pci_interrupt_handler, 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;
+
+	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)
 {
@@ -1152,8 +1199,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;
@@ -1192,7 +1238,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

Re: [PATCH v2] rtw88: pci: enable MSI interrupt

[Daniel Drake]

> +	rtw_pci_disable_interrupt(rtwdev, rtwpci);

I checked the discussion on the v1 patch thread but I still don't follow
this.

You're worried about the case where we're inside the interrupt handler and:
 1. We read the interrupt status to note what needs to be done
 2. <another interrupt arrives here, requiring other work to be done>
 3. We clear the interrupt status bits
 4. We proceed to handle the interrupt but missing any work requested by
    the interrupt in step 2.

Is that right?

I'm not an expert here, but I don't think this is something that drivers
have to worry about. Surely the interrupt controller can be expected to
have a mechanism to "queue up" any interrupt that arrives while an
interrupt is being handled? Otherwise handling of all types of
edge-triggered interrupts (not just MSI) would be overly painful across the
board.

See e.g. https://patchwork.kernel.org/patch/3333681/ as a reference for
what correct interrupt controller behaviour should look like.

> +		ret = pci_enable_msi(pdev);

pci_enable_msi() is "deprecated, don't use"

Thanks
Daniel

Re: [PATCH v2] rtw88: pci: enable MSI interrupt

[Ján Veselý]

On Fri, Aug 23, 2019 at 2:37 AM Daniel Drake <drake@endlessm.com> wrote:
>
> > +     rtw_pci_disable_interrupt(rtwdev, rtwpci);
>
> I checked the discussion on the v1 patch thread but I still don't follow
> this.
>
> You're worried about the case where we're inside the interrupt handler and:
>  1. We read the interrupt status to note what needs to be done
>  2. <another interrupt arrives here, requiring other work to be done>
>  3. We clear the interrupt status bits
>  4. We proceed to handle the interrupt but missing any work requested by
>     the interrupt in step 2.
>
> Is that right?
>
> I'm not an expert here, but I don't think this is something that drivers
> have to worry about. Surely the interrupt controller can be expected to
> have a mechanism to "queue up" any interrupt that arrives while an
> interrupt is being handled? Otherwise handling of all types of
> edge-triggered interrupts (not just MSI) would be overly painful across the
> board.

That's my understanding as well.
entering the interrupt vector clears the IFLAG, so any interrupt will
wait until the IFLAG is restored, or delivered to a different CPU.
wouldn't it be safer to enable interrupts only _after_ registering the
handler in the "rtw_pci_request_irq" function?

regards,
Jan


>
> See e.g. https://patchwork.kernel.org/patch/3333681/ as a reference for
> what correct interrupt controller behaviour should look like.
>
> > +             ret = pci_enable_msi(pdev);
>
> pci_enable_msi() is "deprecated, don't use"
>
> Thanks
> Daniel
>

RE: [PATCH v2] rtw88: pci: enable MSI interrupt

[Tony Chuang]

Hi Daniel and Ján,


> From: Ján Veselý > Sent: Friday, August 23, 2019 3:22 PM
> On Fri, Aug 23, 2019 at 2:37 AM Daniel Drake <drake@endlessm.com> wrote:
> >
> > > +     rtw_pci_disable_interrupt(rtwdev, rtwpci);
> >
> > I checked the discussion on the v1 patch thread but I still don't follow
> > this.
> >
> > You're worried about the case where we're inside the interrupt handler and:
> >  1. We read the interrupt status to note what needs to be done
> >  2. <another interrupt arrives here, requiring other work to be done>
> >  3. We clear the interrupt status bits
> >  4. We proceed to handle the interrupt but missing any work requested by
> >     the interrupt in step 2.
> >
> > Is that right?
> >
> > I'm not an expert here, but I don't think this is something that drivers
> > have to worry about. Surely the interrupt controller can be expected to
> > have a mechanism to "queue up" any interrupt that arrives while an
> > interrupt is being handled? Otherwise handling of all types of
> > edge-triggered interrupts (not just MSI) would be overly painful across the
> > board.
> 
> That's my understanding as well.
> entering the interrupt vector clears the IFLAG, so any interrupt will
> wait until the IFLAG is restored, or delivered to a different CPU.
> wouldn't it be safer to enable interrupts only _after_ registering the
> handler in the "rtw_pci_request_irq" function?
> 
> regards,
> Jan


Yes that's not something drivers need to care about. But I think it is
Because there's a race condition between SW/HW when clearing the ISR.
If interrupt comes after reading ISR and before write-1-clear, the interrupt
controller would have interrupt status raised, and never issue interrupt
signal to host when other new interrupts status are raised.

To avoid this, driver requires to protect the ISR write-1-clear process by
disabling the IMR.


> 
> 
> >
> > See e.g. https://patchwork.kernel.org/patch/3333681/ as a reference for
> > what correct interrupt controller behaviour should look like.
> >
> > > +             ret = pci_enable_msi(pdev);
> >
> > pci_enable_msi() is "deprecated, don't use"

Do you mean I should remove this?
But I cannot find another proper way to enable the MSI.
Maybe pci_alloc_irq_vectors() could work but I am not sure if
It is recommended.


Yan-Hsuan

Re: [PATCH v2] rtw88: pci: enable MSI interrupt

[Jian-Hong Pan]

Tony Chuang <yhchuang@realtek.com> 於 2019年8月27日 週二 下午8:12寫道：
>
> Hi Daniel and Ján,
>
>
> > From: Ján Veselý > Sent: Friday, August 23, 2019 3:22 PM
> > On Fri, Aug 23, 2019 at 2:37 AM Daniel Drake <drake@endlessm.com> wrote:
> > >
> > > > +     rtw_pci_disable_interrupt(rtwdev, rtwpci);
> > >
> > > I checked the discussion on the v1 patch thread but I still don't follow
> > > this.
> > >
> > > You're worried about the case where we're inside the interrupt handler and:
> > >  1. We read the interrupt status to note what needs to be done
> > >  2. <another interrupt arrives here, requiring other work to be done>
> > >  3. We clear the interrupt status bits
> > >  4. We proceed to handle the interrupt but missing any work requested by
> > >     the interrupt in step 2.
> > >
> > > Is that right?
> > >
> > > I'm not an expert here, but I don't think this is something that drivers
> > > have to worry about. Surely the interrupt controller can be expected to
> > > have a mechanism to "queue up" any interrupt that arrives while an
> > > interrupt is being handled? Otherwise handling of all types of
> > > edge-triggered interrupts (not just MSI) would be overly painful across the
> > > board.
> >
> > That's my understanding as well.
> > entering the interrupt vector clears the IFLAG, so any interrupt will
> > wait until the IFLAG is restored, or delivered to a different CPU.
> > wouldn't it be safer to enable interrupts only _after_ registering the
> > handler in the "rtw_pci_request_irq" function?
> >
> > regards,
> > Jan
>
>
> Yes that's not something drivers need to care about. But I think it is
> Because there's a race condition between SW/HW when clearing the ISR.
> If interrupt comes after reading ISR and before write-1-clear, the interrupt
> controller would have interrupt status raised, and never issue interrupt
> signal to host when other new interrupts status are raised.
>
> To avoid this, driver requires to protect the ISR write-1-clear process by
> disabling the IMR.
>
>
> >
> >
> > >
> > > See e.g. https://patchwork.kernel.org/patch/3333681/ as a reference for
> > > what correct interrupt controller behaviour should look like.
> > >
> > > > +             ret = pci_enable_msi(pdev);
> > >
> > > pci_enable_msi() is "deprecated, don't use"
>
> Do you mean I should remove this?
> But I cannot find another proper way to enable the MSI.
> Maybe pci_alloc_irq_vectors() could work but I am not sure if
> It is recommended.

According to the kernel documentation "The MSI Driver Guide HOWTO",
pci_alloc_irq_vectors, pci_irq_vector and pci_free_irq_vectors are the
functions.
https://elixir.bootlin.com/linux/v5.3-rc6/source/Documentation/PCI/msi-howto.rst

Here is an example in r8169 module.
https://elixir.bootlin.com/linux/v5.3-rc6/source/drivers/net/ethernet/realtek/r8169_main.c#L6603

Jian-Hong Pan

Re: [PATCH v2] rtw88: pci: enable MSI interrupt

[Daniel Drake]

On Tue, Aug 27, 2019 at 8:11 PM Tony Chuang <yhchuang@realtek.com> wrote:
> Because there's a race condition between SW/HW when clearing the ISR.
> If interrupt comes after reading ISR and before write-1-clear, the interrupt
> controller would have interrupt status raised, and never issue interrupt
> signal to host when other new interrupts status are raised.
>
> To avoid this, driver requires to protect the ISR write-1-clear process by
> disabling the IMR.

Just to be clear with an example of two interrupts in quick
succession, first ROK then MGNTDOK. I would expect the hardware to
behave as follows:

1. Interrupts are enabled in HIMR reg. MSI is in use.
--- Hardware needs to flag RX condition. It sets IMR_ROK flag in HISR,
then raises a MSI interrupt.
--- Interrupt controller receives this and begins handling it:
2. rtw88 interrupt handler begins execution
3. rtw_pci_irq_recognized()  reads HISR values. ROK is the only bit set.
--- Hardware needs to flag MGNTDOK condition, so it sets the MGNTDOK
bit in HISR and raises another interrupt.
--- Interrupt controller queues this interrupt since it's in the
middle of handling the original ROK interrupt.
4. Back in rtw_pci_irq_recognized(), the HISR values from earlier are
written back to clear them. In this case just the ROK bit is written
back to unset the interrupt. MGNTDOK interrupt is left pending (not
yet noticed by the driver)
5. Interrupt handler continues & finishes handling the RX event.
6. With the first interrupt done, interrupt controller handles the
queued interrupt and causes rtw88 interrupt handler to execute again
7. rtw88 interrupt handler handles and MGNTDOK interrupt.


But are you saying it does not do this, and the behaviour (without
your change) is instead:

1. Interrupts are enabled in HIMR reg. MSI is in use.
--- Hardware needs to flag RX condition. It sets IMR_ROK flag in HISR,
then raises a MSI interrupt.
--- Interrupt controller receives this and begins handling it:
2. rtw88 interrupt handler begins execution
3. rtw_pci_irq_recognized()  reads HISR values. ROK is the only bit set.
--- Hardware needs to flag MGNTDOK condition, so it sets the MGNTDOK
bit in HISR. However it does NOT raise an interrupt since other bits
in HISR were still set at this time.
4. Back in rtw_pci_irq_recognized(), the HISR values from earlier are
written back. In this case just the ROK bit is written back to unset
the interrupt. MGNTDOK interrupt is left pending (not yet noticed by
the driver)
5. Interrupt handler continues & finishes handling the RX event.
6. MGNTDOK interrupt is left pending, and no more interrupts will be
generated by the hardware because even if it sets more HISR bits, the
MGNTDOK bit was already set so it doesn't raise more interrupts.

i.e. you're effectively saying that the hardware will *only* generate
an interrupt when *all* HISR bits were zero immediately before the new
interrupt HISR bit is set?


And then with your change applied it would look like this:

1. Interrupts are enabled in HIMR reg. MSI is in use.
--- Hardware needs to flag RX condition. It sets IMR_ROK flag in HISR,
then raises a MSI interrupt.
--- Interrupt controller receives this and begins handling it:
2. rtw88 interrupt handler begins execution
3. Interrupts are disabled in HIMR reg.
4. rtw_pci_irq_recognized()  reads HISR values. ROK is the only bit set.
--- Hardware needs to flag MGNTDOK condition, however because
interrupts are disabled in HIMR, it simply queues this condition
internally, without affecting HISR values, without generating another
interrupt.
4. Back in rtw_pci_irq_recognized(), the HISR values from earlier are
written back. In this case just the ROK bit is written back to unset
the interrupt. HISR is now value 0.
5. Interrupt handler handles the RX event.
6. Interrupt handler re-enables interrupts in HIMR just before finishing.
--- In response, hardware checks its internal queue and realises that
a MGNTDOK condition is pending. It sets the MGNTDOK bit in HISR and
raises a new interrupt.

Is that right? It seems like strange behaviour on the hardware side.

> Do you mean I should remove this?
> But I cannot find another proper way to enable the MSI.
> Maybe pci_alloc_irq_vectors() could work but I am not sure if
> It is recommended.

Yes, I think you should switch to pci_alloc_irq_vectors() which is the
recommended, documented way.

Thanks,
Daniel

RE: [PATCH v2] rtw88: pci: enable MSI interrupt

[Tony Chuang]

Hi,

A few scattered comments below.

> From: Daniel Drake [mailto:drake@endlessm.com]
> 
> On Tue, Aug 27, 2019 at 8:11 PM Tony Chuang <yhchuang@realtek.com>
> wrote:
> > Because there's a race condition between SW/HW when clearing the ISR.
> > If interrupt comes after reading ISR and before write-1-clear, the interrupt
> > controller would have interrupt status raised, and never issue interrupt
> > signal to host when other new interrupts status are raised.
> >
> > To avoid this, driver requires to protect the ISR write-1-clear process by
> > disabling the IMR.
> 
> Just to be clear with an example of two interrupts in quick
> succession, first ROK then MGNTDOK. I would expect the hardware to
> behave as follows:
> 
> 1. Interrupts are enabled in HIMR reg. MSI is in use.
> --- Hardware needs to flag RX condition. It sets IMR_ROK flag in HISR,
> then raises a MSI interrupt.
> --- Interrupt controller receives this and begins handling it:
> 2. rtw88 interrupt handler begins execution
> 3. rtw_pci_irq_recognized()  reads HISR values. ROK is the only bit set.
> --- Hardware needs to flag MGNTDOK condition, so it sets the MGNTDOK
> bit in HISR and raises another interrupt.
> --- Interrupt controller queues this interrupt since it's in the
> middle of handling the original ROK interrupt.
> 4. Back in rtw_pci_irq_recognized(), the HISR values from earlier are
> written back to clear them. In this case just the ROK bit is written
> back to unset the interrupt. MGNTDOK interrupt is left pending (not
> yet noticed by the driver)
> 5. Interrupt handler continues & finishes handling the RX event.
> 6. With the first interrupt done, interrupt controller handles the
> queued interrupt and causes rtw88 interrupt handler to execute again
> 7. rtw88 interrupt handler handles and MGNTDOK interrupt.
> 
> 
> But are you saying it does not do this, and the behaviour (without
> your change) is instead:
> 
> 1. Interrupts are enabled in HIMR reg. MSI is in use.
> --- Hardware needs to flag RX condition. It sets IMR_ROK flag in HISR,
> then raises a MSI interrupt.
> --- Interrupt controller receives this and begins handling it:
> 2. rtw88 interrupt handler begins execution
> 3. rtw_pci_irq_recognized()  reads HISR values. ROK is the only bit set.
> --- Hardware needs to flag MGNTDOK condition, so it sets the MGNTDOK
> bit in HISR. However it does NOT raise an interrupt since other bits
> in HISR were still set at this time.
> 4. Back in rtw_pci_irq_recognized(), the HISR values from earlier are
> written back. In this case just the ROK bit is written back to unset
> the interrupt. MGNTDOK interrupt is left pending (not yet noticed by
> the driver)
> 5. Interrupt handler continues & finishes handling the RX event.
> 6. MGNTDOK interrupt is left pending, and no more interrupts will be
> generated by the hardware because even if it sets more HISR bits, the
> MGNTDOK bit was already set so it doesn't raise more interrupts.
> 
> i.e. you're effectively saying that the hardware will *only* generate
> an interrupt when *all* HISR bits were zero immediately before the new
> interrupt HISR bit is set?

Yes, that's what I am saying about. I know it looks strange, but I think
this is Realtek's design. And as far as I know this has been used for 9-10
years.

> 
> 
> And then with your change applied it would look like this:
> 
> 1. Interrupts are enabled in HIMR reg. MSI is in use.
> --- Hardware needs to flag RX condition. It sets IMR_ROK flag in HISR,
> then raises a MSI interrupt.
> --- Interrupt controller receives this and begins handling it:
> 2. rtw88 interrupt handler begins execution
> 3. Interrupts are disabled in HIMR reg.
> 4. rtw_pci_irq_recognized()  reads HISR values. ROK is the only bit set.
> --- Hardware needs to flag MGNTDOK condition, however because
> interrupts are disabled in HIMR, it simply queues this condition
> internally, without affecting HISR values, without generating another
> interrupt.

This might be a little different here, I think the MGNTDOK flag is still set.
But new interrupt will not be generated, until HIMR is re-enabled, and I
think re-enabling the HIMR could trigger the hardware to check if any
HISR bit is set, and then generate interrupt.

> 4. Back in rtw_pci_irq_recognized(), the HISR values from earlier are
> written back. In this case just the ROK bit is written back to unset
> the interrupt. HISR is now value 0.
> 5. Interrupt handler handles the RX event.
> 6. Interrupt handler re-enables interrupts in HIMR just before finishing.
> --- In response, hardware checks its internal queue and realises that
> a MGNTDOK condition is pending. It sets the MGNTDOK bit in HISR and
> raises a new interrupt.
> 
> Is that right? It seems like strange behaviour on the hardware side.
> 

Indeed it is a little strange but it works like that, if I don't disable HIMR,
interrupt could be lost and never gets into interrupt handler again.
If so, the interrupt will be stopped, and TX/RX path will be freezed.

> 
> Thanks,
> Daniel
> 

Thanks,
Yan-Hsuan

Re: [PATCH v2] rtw88: pci: enable MSI interrupt

[Daniel Drake]

On Mon, Sep 2, 2019 at 11:02 AM Tony Chuang <yhchuang@realtek.com> wrote:
> Indeed it is a little strange but it works like that, if I don't disable HIMR,
> interrupt could be lost and never gets into interrupt handler again.
> If so, the interrupt will be stopped, and TX/RX path will be freezed.

OK, thanks for checking & explaining.

It's a bit hard to put in words as we have seen, but it would be good
if you could try to summarise this unusual design in the commit
message or in a code comment.

Thanks!
Daniel