Re: [PATCH net-next] r8169: improve interrupt handling
Am 24.02.2018 um 02:35 schrieb Francois Romieu:
> Heiner Kallweit :
> [...]
>> 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/
>
Re: [PATCH net-next] r8169: improve interrupt handling
Heiner Kallweit :
[...]
> 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/
--
Ueimor
[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
---
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 */
