Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 30 Jul 2019 12:57:05 -0700
From:   Brian Norris <briannorris@chromium.org>
To:     yhchuang@realtek.com
Cc:     kvalo@codeaurora.org, linux-wireless@vger.kernel.org,
        jano.vesely@gmail.com
Subject: Re: [PATCH] rtw88: pci: enable MSI interrupt
Message-ID: <20190730195703.GA224792@google.com>
References: <1564487414-9615-1-git-send-email-yhchuang@realtek.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1564487414-9615-1-git-send-email-yhchuang@realtek.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

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>