| Message ID | 7f1cbc71-9d2d-627f-9e0e-b7e7b1f6ce9b@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next,v2] r8169: improve interrupt handling | expand |
From: Heiner Kallweit <hkallweit1@gmail.com> Date: Sat, 24 Feb 2018 16:53:23 +0100 > @@ -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 { ... > + if (tp->mac_version <= RTL_GIGA_MAC_VER_06) { Please, if you are going to keep the logic the same for the older chips, just keep the RTL_FEATURE_MSI flag around instead of adding new (and potentially regression causing) tests for this condition. Thank you.
Am 26.02.2018 um 19:56 schrieb David Miller: > From: Heiner Kallweit <hkallweit1@gmail.com> > Date: Sat, 24 Feb 2018 16:53:23 +0100 > >> @@ -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 { > ... >> + if (tp->mac_version <= RTL_GIGA_MAC_VER_06) { > > Please, if you are going to keep the logic the same for the older > chips, just keep the RTL_FEATURE_MSI flag around instead of adding > new (and potentially regression causing) tests for this condition. > I see your point. In the case here the condition is meant to be true for chip versions: - having the MSIEnable bit - being PCI, not PCIe Both is true for chip versions <= 06 only, as can be seen in different places in the driver, e.g. - where bit MSIEnable is defined comment says: /* 8169 only. Reserved in the 8168. */ - array rtl_chip_infos[] definition shows that only versions <= 06 are named RTL8169xx and are marked as PCI Last but not least condition "chip version <= 06" is used also in other places in the driver when it's about the RTL8169xx PCI chips. At least I'm convinced this gives enough confidence that we can get rid of flag RTL_FEATURE_MSI. > Thank you. >
From: Heiner Kallweit <hkallweit1@gmail.com> Date: Mon, 26 Feb 2018 20:50:32 +0100 > Am 26.02.2018 um 19:56 schrieb David Miller: >> From: Heiner Kallweit <hkallweit1@gmail.com> >> Date: Sat, 24 Feb 2018 16:53:23 +0100 >> >>> @@ -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 { >> ... >>> + if (tp->mac_version <= RTL_GIGA_MAC_VER_06) { >> >> Please, if you are going to keep the logic the same for the older >> chips, just keep the RTL_FEATURE_MSI flag around instead of adding >> new (and potentially regression causing) tests for this condition. >> > I see your point. In the case here the condition is meant to be true > for chip versions: > - having the MSIEnable bit > - being PCI, not PCIe > > Both is true for chip versions <= 06 only, as can be seen in different > places in the driver, e.g. > - where bit MSIEnable is defined comment says: /* 8169 only. Reserved in the 8168. */ > - array rtl_chip_infos[] definition shows that only versions <= 06 > are named RTL8169xx and are marked as PCI > > Last but not least condition "chip version <= 06" is used also in > other places in the driver when it's about the RTL8169xx PCI chips. > > At least I'm convinced this gives enough confidence that we can get > rid of flag RTL_FEATURE_MSI. Fair enough, I'm convinced too. I'll apply this. Meanwhile there is only one flag bit left, and you can therefore convert the flags to a straight "bool has_gmii" or similar. Thanks.
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index 96db3283e..cc51286ee 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,26 @@ 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 int rtl_alloc_irq(struct rtl8169_private *tp) { void __iomem *ioaddr = tp->mmio_addr; - unsigned msi = 0; - u8 cfg2; + unsigned int flags; - 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; - } + if (tp->mac_version <= RTL_GIGA_MAC_VER_06) { + RTL_W8(Cfg9346, Cfg9346_Unlock); + RTL_W8(Config2, RTL_R8(Config2) & ~MSIEnable); + RTL_W8(Cfg9346, Cfg9346_Lock); + flags = PCI_IRQ_LEGACY; + } else { + flags = PCI_IRQ_ALL_TYPES; } - if (tp->mac_version <= RTL_GIGA_MAC_VER_06) - RTL_W8(Config2, cfg2); - return msi; + + return pci_alloc_irq_vectors(tp->pci_dev, 1, 1, flags); } DECLARE_RTL_COND(rtl_link_list_ready_cond) @@ -8497,9 +8489,11 @@ 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); + rc = rtl_alloc_irq(tp); + if (rc < 0) { + netif_err(tp, probe, dev, "Can't allocate interrupt\n"); + return rc; + } /* 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 - remove some dead code, intentionally disabling (unreliable) MSI being partially available on old PCI chips. The patch works fine on a RTL8168evl (chip version 34) and on a RTL8169SB (chip version 04). Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- v2: - disable MSI on old PCI chips even if they partially support it - improve error handling --- drivers/net/ethernet/realtek/r8169.c | 48 ++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 27 deletions(-)