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
Date: Tue, 30 Jul 2019 12:57:05 -0700 [thread overview]
Message-ID: <20190730195703.GA224792@google.com> (raw)
In-Reply-To: <1564487414-9615-1-git-send-email-yhchuang@realtek.com>
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>
next prev parent reply other threads:[~2019-07-30 19:57 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-30 11:50 [PATCH] rtw88: pci: enable MSI interrupt yhchuang
2019-07-30 19:57 ` Brian Norris [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190730195703.GA224792@google.com \
--to=briannorris@chromium.org \
--cc=jano.vesely@gmail.com \
--cc=kvalo@codeaurora.org \
--cc=linux-wireless@vger.kernel.org \
--cc=yhchuang@realtek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.