[PATCH net-next] r8169: improve interrupt handling

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(-)

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);

-- 
2.16.2

Heiner Kallweit [off-list ref] :
[...]

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

[...]

[...]

-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;

[...]

Happily proceeding after error. :o/

-- 
Ueimor

Am 24.02.2018 um 02:35 schrieb Francois Romieu:

Heiner Kallweit [off-list ref] :
[...]

quoted

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.

quoted

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.

[...]

quoted

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

[...]

quoted

-static unsigned rtl_try_msi(struct rtl8169_private *tp,
-			    const struct rtl_cfg_info *cfg)
+static void rtl_alloc_irq(struct rtl8169_private *tp)
 {

[...]

quoted

+	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;

[...]

quoted

@@ -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/