Re: [PATCH] r8169: Fix irq masking in rtl8169_interrupt()

Re: [PATCH] r8169: Fix irq masking in rtl8169_interrupt()

[Jeff Garzik]

Karsten Wiese wrote:
> Only adjust the NAPI bits of the interruptmask, if netif_rx_schedule_prep()
> returns true.
> Without this, I see interrupt storms here using 2.6.29-rcx and 2.6.27.19 and
> device needed rmmod r8169 + modprobe r8169 to start working.
> 
> Signed-off-by: Karsten Wiese <fzu@wemgehoertderstaat.de>
> ---
>  drivers/net/r8169.c |    9 ++++-----
>  1 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
> index b347340..0c943ba 100644
> --- a/drivers/net/r8169.c
> +++ b/drivers/net/r8169.c
> @@ -3708,12 +3708,11 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
>  		rtl8169_check_link_status(dev, tp, ioaddr);
>  
>  	if (status & tp->napi_event) {
> -		RTL_W16(IntrMask, tp->intr_event & ~tp->napi_event);
> -		tp->intr_mask = ~tp->napi_event;
> -
> -		if (likely(netif_rx_schedule_prep(&tp->napi)))
> +		if (likely(netif_rx_schedule_prep(&tp->napi))) {
> +			RTL_W16(IntrMask, tp->intr_event & ~tp->napi_event);
> +			tp->intr_mask = ~tp->napi_event;
>  			__netif_rx_schedule(&tp->napi);
> -		else if (netif_msg_intr(tp)) {
> +		} else if (netif_msg_intr(tp)) {
>  			printk(KERN_INFO "%s: interrupt %04x in poll\n",


FWIW, you should CC net driver patches to netdev@vger.kernel.org, so 
that they get tracked in patchworks and handled by the right people.

	Jeff

[PATCH] r8169: Fix irq masking in rtl8169_interrupt()

[Karsten Wiese]

Only adjust the NAPI bits of the interruptmask, if netif_rx_schedule_prep()
returns true.
Without this, I see interrupt storms here using 2.6.29-rcx and 2.6.27.19 and
device needed rmmod r8169 + modprobe r8169 to start working.

Signed-off-by: Karsten Wiese <fzu@wemgehoertderstaat.de>
---
 drivers/net/r8169.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index b347340..0c943ba 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -3708,12 +3708,11 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
 		rtl8169_check_link_status(dev, tp, ioaddr);
 
 	if (status & tp->napi_event) {
-		RTL_W16(IntrMask, tp->intr_event & ~tp->napi_event);
-		tp->intr_mask = ~tp->napi_event;
-
-		if (likely(netif_rx_schedule_prep(&tp->napi)))
+		if (likely(netif_rx_schedule_prep(&tp->napi))) {
+			RTL_W16(IntrMask, tp->intr_event & ~tp->napi_event);
+			tp->intr_mask = ~tp->napi_event;
 			__netif_rx_schedule(&tp->napi);
-		else if (netif_msg_intr(tp)) {
+		} else if (netif_msg_intr(tp)) {
 			printk(KERN_INFO "%s: interrupt %04x in poll\n",
 			       dev->name, status);
 		}
-- 
1.6.0.6

Re: [PATCH] r8169: Fix irq masking in rtl8169_interrupt()

[Francois Romieu]

Karsten Wiese <fzu@wemgehoertderstaat.de> :
> 
> Only adjust the NAPI bits of the interruptmask, if netif_rx_schedule_prep()
> returns true.
> Without this, I see interrupt storms here using 2.6.29-rcx and 2.6.27.19 and
> device needed rmmod r8169 + modprobe r8169 to start working.

How the patch is supposed to work is not completely clear to me.

Can you elaborate a bit ?

-- 
Ueimor

Re: [PATCH] r8169: Fix irq masking in rtl8169_interrupt()

[Karsten Wiese]

Am Dienstag 17 März 2009 schrieb Francois Romieu:
> Karsten Wiese <fzu@wemgehoertderstaat.de> :
> > 
> > Only adjust the NAPI bits of the interruptmask, if netif_rx_schedule_prep()
> > returns true.
> > Without this, I see interrupt storms here using 2.6.29-rcx and 2.6.27.19 and
> > device needed rmmod r8169 + modprobe r8169 to start working.
> 
> How the patch is supposed to work is not completely clear to me.

Without it, if netif_rx_schedule_prep() returns 0, __netif_rx_schedule() is not
called so nothing is there to care for the transfer. Nonetheless intr_mask is 
changed so that its not possible that __netif_rx_schedule() is ever called
later.

Patch works by letting rtl8169_interrupt() again call netif_rx_schedule_prep()
until it succeeds and __netif_rx_schedule() is called.

Thanks,
Karsten

Re: [PATCH] r8169: Fix irq masking in rtl8169_interrupt()

[Francois Romieu]

Karsten Wiese <fzu@wemgehoertderstaat.de> :
> Am Dienstag 17 März 2009 schrieb Francois Romieu:
[...]
> > How the patch is supposed to work is not completely clear to me.
> 
> Without it, if netif_rx_schedule_prep() returns 0, __netif_rx_schedule() is not
> called so nothing is there to care for the transfer. Nonetheless intr_mask is 
> changed so that its not possible that __netif_rx_schedule() is ever called
> later.
> 
> Patch works by letting rtl8169_interrupt() again call netif_rx_schedule_prep()
> until it succeeds and __netif_rx_schedule() is called.

Thanks for the explanation.

If netif_rx_schedule fails after napi is enabled, there is a
racing poll thread to care for the transfer.

Stated differently the bug is noticeable because napi_enable() 
should be called before request_irq() in rtl8169_open, right ?

-- 
Ueimor

Re: [PATCH] r8169: Fix irq masking in rtl8169_interrupt()

[Karsten Wiese]

Am Mittwoch 18 März 2009 schrieb Francois Romieu:
> Karsten Wiese <fzu@wemgehoertderstaat.de> :
> > Am Dienstag 17 März 2009 schrieb Francois Romieu:
> [...]
> > > How the patch is supposed to work is not completely clear to me.
> > 
> > Without it, if netif_rx_schedule_prep() returns 0, __netif_rx_schedule() is not
> > called so nothing is there to care for the transfer. Nonetheless intr_mask is 
> > changed so that its not possible that __netif_rx_schedule() is ever called
> > later.
> > 
> > Patch works by letting rtl8169_interrupt() again call netif_rx_schedule_prep()
> > until it succeeds and __netif_rx_schedule() is called.
> 
> Thanks for the explanation.
> 
> If netif_rx_schedule fails after napi is enabled, there is a
> racing poll thread to care for the transfer.
> 
> Stated differently the bug is noticeable because napi_enable() 
> should be called before request_irq() in rtl8169_open, right ?

Propably yes. That would mean the first interrupt is received before
rtl_hw_start() is called in rtl8169_open. The source of that
interrupt would be something else then?

Thanks,
Karsten

Re: [PATCH] r8169: Fix irq masking in rtl8169_interrupt()

[Francois Romieu]

Karsten Wiese <fzu@wemgehoertderstaat.de> :
> Am Mittwoch 18 März 2009 schrieb Francois Romieu:
[...]
> > If netif_rx_schedule fails after napi is enabled, there is a
> > racing poll thread to care for the transfer.
> > 
> > Stated differently the bug is noticeable because napi_enable() 
> > should be called before request_irq() in rtl8169_open, right ?
> 
> Propably yes. That would mean the first interrupt is received before
> rtl_hw_start() is called in rtl8169_open. The source of that
> interrupt would be something else then?

All we need is thus a shared interrupt racing on a different CPU
with rtl8169_open and some IO versus memory reordering so that
the hardware is started and a change in the status register can
be seen in the IRQ handler before napi_enable/NAPI_STATE_SCHED
escapes from the CPU where rtl8169_open is running...

It does not look like a credible explanation. :o/

-- 
Ueimor

Re: [PATCH] r8169: Fix irq masking in rtl8169_interrupt()

[David Miller]

From: Francois Romieu <romieu@fr.zoreil.com>
Date: Wed, 18 Mar 2009 23:36:29 +0100

> Karsten Wiese <fzu@wemgehoertderstaat.de> :
> > Am Mittwoch 18 März 2009 schrieb Francois Romieu:
> [...]
> > > If netif_rx_schedule fails after napi is enabled, there is a
> > > racing poll thread to care for the transfer.
> > > 
> > > Stated differently the bug is noticeable because napi_enable() 
> > > should be called before request_irq() in rtl8169_open, right ?
> > 
> > Propably yes. That would mean the first interrupt is received before
> > rtl_hw_start() is called in rtl8169_open. The source of that
> > interrupt would be something else then?
> 
> All we need is thus a shared interrupt racing on a different CPU
> with rtl8169_open and some IO versus memory reordering so that
> the hardware is started and a change in the status register can
> be seen in the IRQ handler before napi_enable/NAPI_STATE_SCHED
> escapes from the CPU where rtl8169_open is running...
> 
> It does not look like a credible explanation. :o/

Agreed, looking at how this part of the driver works I can't
see how this condition can arise either.

Re: [PATCH] r8169: Fix irq masking in rtl8169_interrupt()

[Karsten Wiese]

Am Mittwoch 18 März 2009 schrieb Francois Romieu:
> Karsten Wiese <fzu@wemgehoertderstaat.de> :
> > Am Mittwoch 18 März 2009 schrieb Francois Romieu:
> [...]
> > > If netif_rx_schedule fails after napi is enabled, there is a
> > > racing poll thread to care for the transfer.
> > > 
> > > Stated differently the bug is noticeable because napi_enable() 
> > > should be called before request_irq() in rtl8169_open, right ?
> > 
> > Propably yes. That would mean the first interrupt is received before
> > rtl_hw_start() is called in rtl8169_open. The source of that
> > interrupt would be something else then?
> 
> All we need is thus a shared interrupt racing on a different CPU
> with rtl8169_open and some IO versus memory reordering so that
> the hardware is started and a change in the status register can
> be seen in the IRQ handler before napi_enable/NAPI_STATE_SCHED
> escapes from the CPU where rtl8169_open is running...
> 
> It does not look like a credible explanation. :o/

Found it: CONFIG_DEBUG_SHIRQ is set here causing a "test-call" to
rtl8169_interrupt() from irq_request().

$SUBJECT patch is ok, me still thinks. You?

Thanks,
Karsten

Re: [PATCH] r8169: Fix irq masking in rtl8169_interrupt()

[Francois Romieu]

Karsten Wiese <fzu@wemgehoertderstaat.de> :
[...]
> Found it: CONFIG_DEBUG_SHIRQ is set here causing a "test-call" to
> rtl8169_interrupt() from irq_request().
> 
> $SUBJECT patch is ok, me still thinks. You ?

I will have to convince myself that nothing bad can happen in the
window between netif_rx_schedule_prep and RTL_W16. No big deal.

I do not understand why / how the patch works at all : AFAIU, the status
register must contain some napi-related event in order for the patch to
make a difference. That's exactly what rtl8169_irq_mask_and_ack() is
supposed to prevent. -> Does it fail ?

So I'd really like to understand what is going on here.

PS: while I agree on the motivation for CONFIG_DEBUG_SHIRQ, I wonder if
it is 100% safe to run an IRQ handler from a context which does not
adhere to the original semantic. If its {disable/enable}_irq pair races
with a similar pair due to a different driver on the same (shared) irq,
things could imho be unsafe.

-- 
Ueimor

Re: [PATCH] r8169: Fix irq masking in rtl8169_interrupt()

[Karsten Wiese]

Am Donnerstag 19 März 2009 schrieb Francois Romieu:
> Karsten Wiese <fzu@wemgehoertderstaat.de> :
> [...]
> > Found it: CONFIG_DEBUG_SHIRQ is set here causing a "test-call" to
> > rtl8169_interrupt() from irq_request().
> > 
> > $SUBJECT patch is ok, me still thinks. You ?
> 
> I will have to convince myself that nothing bad can happen in the
> window between netif_rx_schedule_prep and RTL_W16. No big deal.

RTL_W16 is a posted write, if it happens, theres no difference if its
posted before or after netif_rx_schedule_prep.
 
> I do not understand why / how the patch works at all : AFAIU, the status
> register must contain some napi-related event in order for the patch to
> make a difference.

It contains 0x25 when the "test-call" runs, Link, Rx and Tx Flags.

> That's exactly what rtl8169_irq_mask_and_ack() is
> supposed to prevent. -> Does it fail ?

rtl8169_irq_mask_and_ack() resets status once and prevents interrupts
being sent by device. Maybe status reset doesn't work because device is
stopped or device isn't _completely_ stopped and updates status flags later.
Maybe Tx status only becomes 0 if some Tx data is queued.
 
> So I'd really like to understand what is going on here.

Well, without patch any further interrupt causing napi-status isn't reset
_and_ netif_rx_schedule() isn't active. screaming interrupts.
slooooowish pc. Wonder why it proceeded at all though.

After rmmod/modprobe the status register contained 0x20 when in
rtl8169_irq_mask_and_ack() during "test-call", letting device work
normally.
 
> PS: while I agree on the motivation for CONFIG_DEBUG_SHIRQ, I wonder if
> it is 100% safe to run an IRQ handler from a context which does not
> adhere to the original semantic. If its {disable/enable}_irq pair races
> with a similar pair due to a different driver on the same (shared) irq,
> things could imho be unsafe.

From the comment of disable_irq():
	"This function waits for any pending IRQ handlers for this interrupt
 	to complete before returning."
No race possible, in theory ;-)

The pc I debugged this on is a UP with no other device active on the used
int16.

Thanks,
Karsten

Re: [PATCH] r8169: Fix irq masking in rtl8169_interrupt()

[Francois Romieu]

Karsten Wiese <fzu@wemgehoertderstaat.de> :
[...]
> The pc I debugged this on is a UP with no other device active on the used
> int16.

/me wonders...

Does your network adapter support pxe or some boot time operation ?

Btw the XID of the device would be welcome.

-- 
Ueimor

Re: [PATCH] r8169: Fix irq masking in rtl8169_interrupt()

[Karsten Wiese]

Am Sonntag 22 März 2009 schrieb Francois Romieu:
> Karsten Wiese <fzu@wemgehoertderstaat.de> :
> [...]
> > The pc I debugged this on is a UP with no other device active on the used
> > int16.
> 
> /me wonders...
> 
> Does your network adapter support pxe or some boot time operation ?

Yes, deavtivated in BIOS.

> Btw the XID of the device would be welcome.

XID is 04000000

Thanks,
Karsten

Re: [PATCH] r8169: Fix irq masking in rtl8169_interrupt()

[Francois Romieu]

Karsten Wiese <fzu@wemgehoertderstaat.de> :
[...]
> Yes, deavtivated in BIOS.

Ok, it starts to make some sense.

Does the patch below make a difference ?

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 4b7cb38..b5a7358 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -2011,8 +2011,6 @@ rtl8169_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		}
 	}
 
-	pci_set_master(pdev);
-
 	/* ioremap MMIO region */
 	ioaddr = ioremap(pci_resource_start(pdev, region), R8169_REGS_SIZE);
 	if (!ioaddr) {
@@ -2039,6 +2037,8 @@ rtl8169_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		msleep_interruptible(1);
 	}
 
+	pci_set_master(pdev);
+
 	/* Identify chip attached to board */
 	rtl8169_get_mac_version(tp, ioaddr);
 
-- 
Ueimor

Re: [PATCH] r8169: Fix irq masking in rtl8169_interrupt()

[Karsten Wiese]

Am Montag 23 März 2009 schrieb Francois Romieu:
> Karsten Wiese <fzu@wemgehoertderstaat.de> :
> [...]
> > Yes, deavtivated in BIOS.
> 
> Ok, it starts to make some sense.
> 
> Does the patch below make a difference ?

No.

> diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
> index 4b7cb38..b5a7358 100644
> --- a/drivers/net/r8169.c
> +++ b/drivers/net/r8169.c
> @@ -2011,8 +2011,6 @@ rtl8169_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		}
>  	}
>  
> -	pci_set_master(pdev);
> -
>  	/* ioremap MMIO region */
>  	ioaddr = ioremap(pci_resource_start(pdev, region), R8169_REGS_SIZE);
>  	if (!ioaddr) {
> @@ -2039,6 +2037,8 @@ rtl8169_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		msleep_interruptible(1);
>  	}
>  
> +	pci_set_master(pdev);
> +
>  	/* Identify chip attached to board */
>  	rtl8169_get_mac_version(tp, ioaddr);
>  

Re: [PATCH] r8169: Fix irq masking in rtl8169_interrupt()

[Karsten Wiese]

Am Dienstag 24 März 2009 schrieb Karsten Wiese:
> Am Montag 23 März 2009 schrieb Francois Romieu:
> > 
> > Does the patch below make a difference ?
> 
> No.

patch below helps.

Thanks,
Karsten
--------------------->
[PATCH] r8169: Reset IntrStatus after chip rest

On a MSI MS-6702E mainboard, when in rtl8169_init_one() for the first time
after BIOS has run, IntrStatus reads 5 after chip has been reset.
IntrStatus should equal 0 there, so patch changes IntrStatus reset to happen
after chip reset instead of before.
R8169_MSG_DEFAULT is changed to include NETIF_MSG_INTR, because without
an important error message from rtl8169_interrupt() isn't shown by default.

Signed-off-by: Karsten Wiese <fzu@wemgehoertderstaat.de>
---
 drivers/net/r8169.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 56a1eb8..b06e153 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -45,8 +45,9 @@
 #define dprintk(fmt, args...)	do {} while (0)
 #endif /* RTL8169_DEBUG */
 
-#define R8169_MSG_DEFAULT \
-	(NETIF_MSG_DRV | NETIF_MSG_PROBE | NETIF_MSG_IFUP | NETIF_MSG_IFDOWN)
+#define R8169_MSG_DEFAULT					\
+	(NETIF_MSG_DRV | NETIF_MSG_PROBE |			\
+	 NETIF_MSG_IFUP | NETIF_MSG_IFDOWN | NETIF_MSG_INTR)
 
 #define TX_BUFFS_AVAIL(tp) \
 	(tp->dirty_tx + NUM_TX_DESC - tp->cur_tx - 1)
@@ -2076,7 +2077,7 @@ rtl8169_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		dev_info(&pdev->dev, "no PCI Express capability\n");
 
 	/* Unneeded ? Don't mess with Mrs. Murphy. */
-	rtl8169_irq_mask_and_ack(ioaddr);
+	RTL_W16(IntrMask, 0x0000);
 
 	/* Soft reset the chip. */
 	RTL_W8(ChipCmd, CmdReset);
@@ -2088,6 +2089,11 @@ rtl8169_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		msleep_interruptible(1);
 	}
 
+	/* On an MSI MS-6702E, when here for the first time after
+	   BIOS has run, IntrStatus contains 5. Play safe.
+	*/
+	RTL_W16(IntrStatus, 0xffff);
+
 	/* Identify chip attached to board */
 	rtl8169_get_mac_version(tp, ioaddr);
 
-- 
1.6.0.6

Re: [PATCH] r8169: Fix irq masking in rtl8169_interrupt()

[Francois Romieu]

Karsten Wiese <fzu@wemgehoertderstaat.de> :
[...]
> patch below helps.

Thanks.

I'll wrap it up without the change to R8169_MSG_DEFAULT:
- it can be set through ethtool and a module option
- the current default is not a clear nuisance

-- 
Ueimor