Linux-Wireless Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] rtw88: pci: enable MSI interrupt
@ 2019-07-30 11:50 yhchuang
  2019-07-30 19:57 ` Brian Norris
  0 siblings, 1 reply; 4+ 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	[flat|nested] 4+ 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; 4+ 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] 4+ 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; 4+ 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] 4+ messages in thread

* Re: [PATCH] rtw88: pci: enable MSI interrupt
  2019-08-01  9:21   ` Tony Chuang
@ 2019-08-08 16:51     ` Brian Norris
  0 siblings, 0 replies; 4+ 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] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ 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

Linux-Wireless Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-wireless/0 linux-wireless/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-wireless linux-wireless/ https://lore.kernel.org/linux-wireless \
		linux-wireless@vger.kernel.org linux-wireless@archiver.kernel.org
	public-inbox-index linux-wireless


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-wireless


AGPL code for this site: git clone https://public-inbox.org/ public-inbox