r8169: improve spurious interrupt detection

r8169: improve spurious interrupt detection

[Heiner Kallweit]

Improve detection of spurious interrupts by checking against the
interrupt mask as currently set in the chip.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 8462553e3..99bc3de90 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -6405,8 +6405,9 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
 {
 	struct rtl8169_private *tp = dev_instance;
 	u16 status = rtl_get_events(tp);
+	u16 irq_mask = RTL_R16(tp, IntrMask);
 
-	if (status == 0xffff || !(status & tp->irq_mask))
+	if (status == 0xffff || !(status & irq_mask))
 		return IRQ_NONE;
 
 	if (unlikely(status & SYSErr)) {
-- 
2.20.0

Re: r8169: improve spurious interrupt detection

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Sat, 15 Dec 2018 16:24:02 +0100

> @@ -6405,8 +6405,9 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
>  {
>  	struct rtl8169_private *tp = dev_instance;
>  	u16 status = rtl_get_events(tp);
> +	u16 irq_mask = RTL_R16(tp, IntrMask);
>  
> -	if (status == 0xffff || !(status & tp->irq_mask))
> +	if (status == 0xffff || !(status & irq_mask))
>  		return IRQ_NONE;

Heiner, this is really expensive.  Why is the tp->irq_mask value out of
sync with the value programmed into IntrMask?  Is there some way we can
fix that instead?

Re: r8169: improve spurious interrupt detection

[Heiner Kallweit]

On 15.12.2018 19:35, David Miller wrote:
> From: Heiner Kallweit <hkallweit1@gmail.com>
> Date: Sat, 15 Dec 2018 16:24:02 +0100
> 
>> @@ -6405,8 +6405,9 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
>>  {
>>  	struct rtl8169_private *tp = dev_instance;
>>  	u16 status = rtl_get_events(tp);
>> +	u16 irq_mask = RTL_R16(tp, IntrMask);
>>  
>> -	if (status == 0xffff || !(status & tp->irq_mask))
>> +	if (status == 0xffff || !(status & irq_mask))
>>  		return IRQ_NONE;
> 
> Heiner, this is really expensive.  Why is the tp->irq_mask value out of
> sync with the value programmed into IntrMask?  Is there some way we can
> fix that instead?
> 
tp->irq_mask holds the chip-specific interrupt mask. It doesn't say
whether interrupts are enabled or not. rtl_get_events() reads via
PCI(e) anyway, so I was under the impression that one more PCI(e) read
doesn't really matter.
We could introduce a flag shadowing the "interrupts are enabled" state
and use it here. But I'm not sure whether it's worth it.
Alternatively we could also go with the readw_relaxed() version to
get the values, this would eliminate the memory barrier at least.

Re: r8169: improve spurious interrupt detection

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Sat, 15 Dec 2018 19:44:35 +0100

> tp->irq_mask holds the chip-specific interrupt mask. It doesn't say
> whether interrupts are enabled or not. rtl_get_events() reads via
> PCI(e) anyway, so I was under the impression that one more PCI(e) read
> doesn't really matter.
> We could introduce a flag shadowing the "interrupts are enabled" state
> and use it here. But I'm not sure whether it's worth it.
> Alternatively we could also go with the readw_relaxed() version to
> get the values, this would eliminate the memory barrier at least.

Thank for explaining, I thought ->irq_mask shadows IntrMask.

I see what you are saying about rtl8169_irq_mask_and_ack() so maybe
adding one more PCIe read won't matter.

I think I'll apply this as-is, this was supposed to be net-next and
that's why you reposted it with a corrected Subject correct?

Re: r8169: improve spurious interrupt detection

[Heiner Kallweit]

On 15.12.2018 20:15, David Miller wrote:
> From: Heiner Kallweit <hkallweit1@gmail.com>
> Date: Sat, 15 Dec 2018 19:44:35 +0100
> 
>> tp->irq_mask holds the chip-specific interrupt mask. It doesn't say
>> whether interrupts are enabled or not. rtl_get_events() reads via
>> PCI(e) anyway, so I was under the impression that one more PCI(e) read
>> doesn't really matter.
>> We could introduce a flag shadowing the "interrupts are enabled" state
>> and use it here. But I'm not sure whether it's worth it.
>> Alternatively we could also go with the readw_relaxed() version to
>> get the values, this would eliminate the memory barrier at least.
> 
> Thank for explaining, I thought ->irq_mask shadows IntrMask.
> 
> I see what you are saying about rtl8169_irq_mask_and_ack() so maybe
> adding one more PCIe read won't matter.
> 
> I think I'll apply this as-is, this was supposed to be net-next and
> that's why you reposted it with a corrected Subject correct?
> 
Right, I missed to state net vs. net-next.
>