| Message ID | 1395660884-15042-1-git-send-email-adam.lee@canonical.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Mon, Mar 24, 2014 at 07:34:44PM +0800, Adam Lee wrote: > Add MSI interrupts support, enable it when msi_support flag is true, > also could fallback to pin-based interrupts mode if MSI mode fails. > > Signed-off-by: Adam Lee <adam.lee@canonical.com> > --- > drivers/net/wireless/rtlwifi/pci.c | 63 ++++++++++++++++++++++++++++++++++++-- > 1 file changed, 61 insertions(+), 2 deletions(-) This is the base of "[PATCH] rtlwifi: rtl8188ee: enable MSI interrupts support" in thread. We met this issue[1] while enabling some HP notebooks, whose wifi hardware modules can't get AP scan results with pin-based interrupts. Submitting these two patches ahead of RealTek's schedule so we can fix it sooner for non-preload distro users. [1] https://bugs.launchpad.net/bugs/1296591
On 03/24/2014 06:34 AM, Adam Lee wrote: > Add MSI interrupts support, enable it when msi_support flag is true, > also could fallback to pin-based interrupts mode if MSI mode fails. > > Signed-off-by: Adam Lee <adam.lee@canonical.com> Your first patch turns on MSI support unconditionally. That implies that there are no harmful effects for attempting to use MSI on a box where it is not needed, and/or not implemented. If that is the case, then the msi_support bool should be eliminated, your first patch dropped, and this one revised appropriately. Driver rtl8723be added this bool, but never uses it. Once that driver reaches mainline, a patch can be prepared to remove the variable. I think this patch should be applied to stable. Applying it back to 3.10+ should be far enough. That will pick up the initial submission of the RTL8188EE driver. Boxes that need MSI interrupts are not likely to use any of the older chips. There are some additional in-line comments below. > --- > drivers/net/wireless/rtlwifi/pci.c | 63 ++++++++++++++++++++++++++++++++++++-- > 1 file changed, 61 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/rtlwifi/pci.c b/drivers/net/wireless/rtlwifi/pci.c > index f26f4ff..dd8497a 100644 > --- a/drivers/net/wireless/rtlwifi/pci.c > +++ b/drivers/net/wireless/rtlwifi/pci.c > @@ -1853,6 +1853,63 @@ static bool _rtl_pci_find_adapter(struct pci_dev *pdev, > return true; > } > > +static int rtl_pci_intr_mode_msi(struct ieee80211_hw *hw) > +{ > + struct rtl_priv *rtlpriv = rtl_priv(hw); > + struct rtl_pci_priv *pcipriv = rtl_pcipriv(hw); > + struct rtl_pci *rtlpci = rtl_pcidev(pcipriv); > + int ret; I like a blank line between the declarations and the code. > + ret = pci_enable_msi(rtlpci->pdev); > + if (ret < 0) > + return ret; > + > + ret = request_irq(rtlpci->pdev->irq, &_rtl_pci_interrupt, > + IRQF_SHARED, KBUILD_MODNAME, hw); > + if (ret < 0) { > + pci_disable_msi(rtlpci->pdev); > + return ret; > + } > + > + rtlpci->using_msi = true; > + > + RT_TRACE(rtlpriv, COMP_INIT|COMP_INTR, DBG_DMESG, > + ("MSI Interrupt Mode!\n")); Doesn't checkpatch.pl complain about the alignment here? > + return 0; > +} > + > +static int rtl_pci_intr_mode_legacy(struct ieee80211_hw *hw) > +{ > + struct rtl_priv *rtlpriv = rtl_priv(hw); > + struct rtl_pci_priv *pcipriv = rtl_pcipriv(hw); > + struct rtl_pci *rtlpci = rtl_pcidev(pcipriv); > + int ret; > + > + ret = request_irq(rtlpci->pdev->irq, &_rtl_pci_interrupt, > + IRQF_SHARED, KBUILD_MODNAME, hw); > + if (ret < 0) > + return ret; > + > + rtlpci->using_msi = false; > + RT_TRACE(rtlpriv, COMP_INIT|COMP_INTR, DBG_DMESG, > + ("Pin-based Interrupt Mode!\n")); > + return 0; > +} > + > +static int rtl_pci_intr_mode_decide(struct ieee80211_hw *hw) > +{ > + struct rtl_pci_priv *pcipriv = rtl_pcipriv(hw); > + struct rtl_pci *rtlpci = rtl_pcidev(pcipriv); > + int ret; > + if (rtlpci->msi_support == true) { As discussed above, this test is not needed. If it were, the current version of checkpatch.pl complains about the "== true" part. > + ret = rtl_pci_intr_mode_msi(hw); > + if (ret < 0) > + ret = rtl_pci_intr_mode_legacy(hw); > + } else { > + ret = rtl_pci_intr_mode_legacy(hw); > + } > + return ret; > +} > + > int rtl_pci_probe(struct pci_dev *pdev, > const struct pci_device_id *id) > { > @@ -1995,8 +2052,7 @@ int rtl_pci_probe(struct pci_dev *pdev, > } > > rtlpci = rtl_pcidev(pcipriv); > - err = request_irq(rtlpci->pdev->irq, &_rtl_pci_interrupt, > - IRQF_SHARED, KBUILD_MODNAME, hw); > + err = rtl_pci_intr_mode_decide(hw); > if (err) { > RT_TRACE(rtlpriv, COMP_INIT, DBG_DMESG, > "%s: failed to register IRQ handler\n", > @@ -2064,6 +2120,9 @@ void rtl_pci_disconnect(struct pci_dev *pdev) > rtlpci->irq_alloc = 0; > } > > + if (rtlpci->using_msi == true) Again "if (rtlpci->using_msi)" is preferred. > + pci_disable_msi(rtlpci->pdev); > + > list_del(&rtlpriv->list); > if (rtlpriv->io.pci_mem_start != 0) { > pci_iounmap(pdev, (void __iomem *)rtlpriv->io.pci_mem_start); > When you resubmit, use "[PATCH V2]" in the subject, and use the proper CC stable with [3.10+] following the address. Larry -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 24, 2014 at 10:14:06AM -0500, Larry Finger wrote: > On 03/24/2014 06:34 AM, Adam Lee wrote: > >Add MSI interrupts support, enable it when msi_support flag is true, > >also could fallback to pin-based interrupts mode if MSI mode fails. > > > >Signed-off-by: Adam Lee <adam.lee@canonical.com> > > Your first patch turns on MSI support unconditionally. That implies that > there are no harmful effects for attempting to use MSI on a box where it is > not needed, and/or not implemented. If that is the case, then the > msi_support bool should be eliminated, your first patch dropped, and this > one revised appropriately. Driver rtl8723be added this bool, but never uses > it. Once that driver reaches mainline, a patch can be prepared to remove the > variable. Hi, Larry Just confirmed with RealTek wireless engineer, their policy is: > Case 1: if the platform supports both MSI and Pin-based, our driver will use MSI. > Case 2: if the platform supports MSI only, our driver will use MSI. > Case 3: if the platform supports Pin-Based only, out driver will use Pin-Based. I think it's safe enough to apply the first "rtlwifi: rtl8188ee: enable MSI interrupts support" patch also, and the msi_support bool should not be eliminated(at least for now) in case some wifi modules don't support MSI well. Thanks.
diff --git a/drivers/net/wireless/rtlwifi/pci.c b/drivers/net/wireless/rtlwifi/pci.c index f26f4ff..dd8497a 100644 --- a/drivers/net/wireless/rtlwifi/pci.c +++ b/drivers/net/wireless/rtlwifi/pci.c @@ -1853,6 +1853,63 @@ static bool _rtl_pci_find_adapter(struct pci_dev *pdev, return true; } +static int rtl_pci_intr_mode_msi(struct ieee80211_hw *hw) +{ + struct rtl_priv *rtlpriv = rtl_priv(hw); + struct rtl_pci_priv *pcipriv = rtl_pcipriv(hw); + struct rtl_pci *rtlpci = rtl_pcidev(pcipriv); + int ret; + ret = pci_enable_msi(rtlpci->pdev); + if (ret < 0) + return ret; + + ret = request_irq(rtlpci->pdev->irq, &_rtl_pci_interrupt, + IRQF_SHARED, KBUILD_MODNAME, hw); + if (ret < 0) { + pci_disable_msi(rtlpci->pdev); + return ret; + } + + rtlpci->using_msi = true; + + RT_TRACE(rtlpriv, COMP_INIT|COMP_INTR, DBG_DMESG, + ("MSI Interrupt Mode!\n")); + return 0; +} + +static int rtl_pci_intr_mode_legacy(struct ieee80211_hw *hw) +{ + struct rtl_priv *rtlpriv = rtl_priv(hw); + struct rtl_pci_priv *pcipriv = rtl_pcipriv(hw); + struct rtl_pci *rtlpci = rtl_pcidev(pcipriv); + int ret; + + ret = request_irq(rtlpci->pdev->irq, &_rtl_pci_interrupt, + IRQF_SHARED, KBUILD_MODNAME, hw); + if (ret < 0) + return ret; + + rtlpci->using_msi = false; + RT_TRACE(rtlpriv, COMP_INIT|COMP_INTR, DBG_DMESG, + ("Pin-based Interrupt Mode!\n")); + return 0; +} + +static int rtl_pci_intr_mode_decide(struct ieee80211_hw *hw) +{ + struct rtl_pci_priv *pcipriv = rtl_pcipriv(hw); + struct rtl_pci *rtlpci = rtl_pcidev(pcipriv); + int ret; + if (rtlpci->msi_support == true) { + ret = rtl_pci_intr_mode_msi(hw); + if (ret < 0) + ret = rtl_pci_intr_mode_legacy(hw); + } else { + ret = rtl_pci_intr_mode_legacy(hw); + } + return ret; +} + int rtl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) { @@ -1995,8 +2052,7 @@ int rtl_pci_probe(struct pci_dev *pdev, } rtlpci = rtl_pcidev(pcipriv); - err = request_irq(rtlpci->pdev->irq, &_rtl_pci_interrupt, - IRQF_SHARED, KBUILD_MODNAME, hw); + err = rtl_pci_intr_mode_decide(hw); if (err) { RT_TRACE(rtlpriv, COMP_INIT, DBG_DMESG, "%s: failed to register IRQ handler\n", @@ -2064,6 +2120,9 @@ void rtl_pci_disconnect(struct pci_dev *pdev) rtlpci->irq_alloc = 0; } + if (rtlpci->using_msi == true) + pci_disable_msi(rtlpci->pdev); + list_del(&rtlpriv->list); if (rtlpriv->io.pci_mem_start != 0) { pci_iounmap(pdev, (void __iomem *)rtlpriv->io.pci_mem_start);
Add MSI interrupts support, enable it when msi_support flag is true, also could fallback to pin-based interrupts mode if MSI mode fails. Signed-off-by: Adam Lee <adam.lee@canonical.com> --- drivers/net/wireless/rtlwifi/pci.c | 63 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 61 insertions(+), 2 deletions(-)