| Message ID | 25f6212f-5c76-365f-79a2-faae269877d7@gmail.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] r8169: improve interrupt handling | expand |
Heiner Kallweit <hkallweit1@gmail.com> : [...] > Last but not least it enables a feature which was (I presume accidently) > disabled before. There are members of the RTL8169 family supporting MSI > (e.g. RTL8169SB), however MSI never got enabled because RTL_CFG_0 was > missing flag RTL_FEATURE_MSI. > An indicator for "accidently" is that statement "cfg2 |= MSIEnable;" The reality is more simple: it could had been removed. > in rtl_try_msi() is dead code. cfg2 is used for chip versions <= 06 > only and for all these chip versions RTL_FEATURE_MSI isn't set. On purpose: 1. mostly untested 2. MSI without MSI-X does not buy much 3. wrt 2., ok, it kills (yucky) plain old shared PCI irq (remember those ?) but I didn't end feeling that good about realtek MSI support on older chipsets to enable it any further [...] > diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c > index 96db3283e..4730db990 100644 > --- a/drivers/net/ethernet/realtek/r8169.c > +++ b/drivers/net/ethernet/realtek/r8169.c [...] > -static unsigned rtl_try_msi(struct rtl8169_private *tp, > - const struct rtl_cfg_info *cfg) > +static void rtl_alloc_irq(struct rtl8169_private *tp) > { [...] > + ret = pci_alloc_irq_vectors(tp->pci_dev, 1, 1, PCI_IRQ_ALL_TYPES); > + if (ret < 0) { > + netif_err(tp, drv, tp->dev, "failed to allocate irq!\n"); > + return; [...] > @@ -8497,9 +8495,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > chipset = tp->mac_version; > tp->txd_version = rtl_chip_infos[chipset].txd_version; > > - RTL_W8(Cfg9346, Cfg9346_Unlock); > - tp->features |= rtl_try_msi(tp, cfg); > - RTL_W8(Cfg9346, Cfg9346_Lock); > + rtl_alloc_irq(tp); Happily proceeding after error. :o/
Am 24.02.2018 um 02:35 schrieb Francois Romieu: > Heiner Kallweit <hkallweit1@gmail.com> : > [...] >> Last but not least it enables a feature which was (I presume accidently) >> disabled before. There are members of the RTL8169 family supporting MSI >> (e.g. RTL8169SB), however MSI never got enabled because RTL_CFG_0 was >> missing flag RTL_FEATURE_MSI. >> An indicator for "accidently" is that statement "cfg2 |= MSIEnable;" > > The reality is more simple: it could had been removed. > >> in rtl_try_msi() is dead code. cfg2 is used for chip versions <= 06 >> only and for all these chip versions RTL_FEATURE_MSI isn't set. > > On purpose: > 1. mostly untested > 2. MSI without MSI-X does not buy much > 3. wrt 2., ok, it kills (yucky) plain old shared PCI irq (remember those ?) > but I didn't end feeling that good about realtek MSI support on older > chipsets to enable it any further > Good to know, thanks for the feedback. Then I'll change the patch to leave MSI disabled on old PCI chips. Plus addressing your last comment. > [...] >> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c >> index 96db3283e..4730db990 100644 >> --- a/drivers/net/ethernet/realtek/r8169.c >> +++ b/drivers/net/ethernet/realtek/r8169.c > [...] >> -static unsigned rtl_try_msi(struct rtl8169_private *tp, >> - const struct rtl_cfg_info *cfg) >> +static void rtl_alloc_irq(struct rtl8169_private *tp) >> { > [...] >> + ret = pci_alloc_irq_vectors(tp->pci_dev, 1, 1, PCI_IRQ_ALL_TYPES); >> + if (ret < 0) { >> + netif_err(tp, drv, tp->dev, "failed to allocate irq!\n"); >> + return; > [...] >> @@ -8497,9 +8495,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) >> chipset = tp->mac_version; >> tp->txd_version = rtl_chip_infos[chipset].txd_version; >> >> - RTL_W8(Cfg9346, Cfg9346_Unlock); >> - tp->features |= rtl_try_msi(tp, cfg); >> - RTL_W8(Cfg9346, Cfg9346_Lock); >> + rtl_alloc_irq(tp); > > Happily proceeding after error. :o/ >
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index 96db3283e..4730db990 100644 --- a/drivers/net/ethernet/realtek/r8169.c +++ b/drivers/net/ethernet/realtek/r8169.c @@ -736,8 +736,7 @@ struct ring_info { }; enum features { - RTL_FEATURE_MSI = (1 << 0), - RTL_FEATURE_GMII = (1 << 1), + RTL_FEATURE_GMII = (1 << 0), }; struct rtl8169_counters { @@ -7847,7 +7846,7 @@ static int rtl8169_close(struct net_device *dev) cancel_work_sync(&tp->wk.work); - free_irq(pdev->irq, dev); + pci_free_irq(pdev, 0, dev); dma_free_coherent(&pdev->dev, R8169_RX_RING_BYTES, tp->RxDescArray, tp->RxPhyAddr); @@ -7903,9 +7902,8 @@ static int rtl_open(struct net_device *dev) rtl_request_firmware(tp); - retval = request_irq(pdev->irq, rtl8169_interrupt, - (tp->features & RTL_FEATURE_MSI) ? 0 : IRQF_SHARED, - dev->name, dev); + retval = pci_request_irq(pdev, 0, rtl8169_interrupt, NULL, dev, + dev->name); if (retval < 0) goto err_release_fw_2; @@ -8253,7 +8251,7 @@ static const struct rtl_cfg_info { .region = 2, .align = 8, .event_slow = SYSErr | LinkChg | RxOverflow, - .features = RTL_FEATURE_GMII | RTL_FEATURE_MSI, + .features = RTL_FEATURE_GMII, .coalesce_info = rtl_coalesce_info_8168_8136, .default_ver = RTL_GIGA_MAC_VER_11, }, @@ -8263,32 +8261,32 @@ static const struct rtl_cfg_info { .align = 8, .event_slow = SYSErr | LinkChg | RxOverflow | RxFIFOOver | PCSTimeout, - .features = RTL_FEATURE_MSI, .coalesce_info = rtl_coalesce_info_8168_8136, .default_ver = RTL_GIGA_MAC_VER_13, } }; /* Cfg9346_Unlock assumed. */ -static unsigned rtl_try_msi(struct rtl8169_private *tp, - const struct rtl_cfg_info *cfg) +static void rtl_alloc_irq(struct rtl8169_private *tp) { void __iomem *ioaddr = tp->mmio_addr; - unsigned msi = 0; u8 cfg2; + int ret; - cfg2 = RTL_R8(Config2) & ~MSIEnable; - if (cfg->features & RTL_FEATURE_MSI) { - if (pci_enable_msi(tp->pci_dev)) { - netif_info(tp, hw, tp->dev, "no MSI. Back to INTx.\n"); - } else { - cfg2 |= MSIEnable; - msi = RTL_FEATURE_MSI; - } + ret = pci_alloc_irq_vectors(tp->pci_dev, 1, 1, PCI_IRQ_ALL_TYPES); + if (ret < 0) { + netif_err(tp, drv, tp->dev, "failed to allocate irq!\n"); + return; } - if (tp->mac_version <= RTL_GIGA_MAC_VER_06) + + if (tp->mac_version <= RTL_GIGA_MAC_VER_06) { + RTL_W8(Cfg9346, Cfg9346_Unlock); + cfg2 = RTL_R8(Config2) & ~MSIEnable; + if (pci_dev_msi_enabled(tp->pci_dev)) + cfg2 |= MSIEnable; RTL_W8(Config2, cfg2); - return msi; + RTL_W8(Cfg9346, Cfg9346_Lock); + } } DECLARE_RTL_COND(rtl_link_list_ready_cond) @@ -8497,9 +8495,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) chipset = tp->mac_version; tp->txd_version = rtl_chip_infos[chipset].txd_version; - RTL_W8(Cfg9346, Cfg9346_Unlock); - tp->features |= rtl_try_msi(tp, cfg); - RTL_W8(Cfg9346, Cfg9346_Lock); + rtl_alloc_irq(tp); /* override BIOS settings, use userspace tools to enable WOL */ __rtl8169_set_wol(tp, 0);
This patch improves few aspects of interrupt handling: - update to current interrupt allocation API (use pci_alloc_irq_vectors() instead of deprecated pci_enable_msi()) - this implicitly will allocate a MSI-X interrupt if available - get rid of flag RTL_FEATURE_MSI Last but not least it enables a feature which was (I presume accidently) disabled before. There are members of the RTL8169 family supporting MSI (e.g. RTL8169SB), however MSI never got enabled because RTL_CFG_0 was missing flag RTL_FEATURE_MSI. An indicator for "accidently" is that statement "cfg2 |= MSIEnable;" in rtl_try_msi() is dead code. cfg2 is used for chip versions <= 06 only and for all these chip versions RTL_FEATURE_MSI isn't set. The patch works fine on a RTL8168evl (chip version 34). The previously accidently disabled MSI support for the few members of the old RTL8169 family supporting MSI isn't tested. The RTL8169SB (chip version 04) I have at least still works in a PCI slot w/o MSI. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/ethernet/realtek/r8169.c | 44 ++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 24 deletions(-)