[PATCH] Disable polling in rtl8169_suspend() to prevent race with rtl8169_interrupt()

[PATCH] Disable polling in rtl8169_suspend() to prevent race with rtl8169_interrupt()

[Karsten Wiese]

Hi,

Sometimes (~ 1 in 10) after suspend to disk my r8169 nic doesn't work and a
"r8169: eth0: link down" gets mingled up with those suspend messages.
First tests show nothing bad, so maybe this patch really helps.
Will post again, if it doesn't.

      Karsten
------------------>

Disable polling in rtl8169_suspend() to prevent race with rtl8169_interrupt()

rtl8169_interrupt() is called during polling.
Without patch, rtl8169_interrupt() could run after rtl8169_asic_down(),
causing a disabled link state being saved later in rtl8169_suspend() by
pci_save_state().
netif_stop_queue() can go as it is called by netif_device_detach().

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

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index c76dd29..fee6c8e 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -3024,7 +3024,7 @@ static int rtl8169_suspend(struct pci_dev *pdev, pm_message_t state)
 		goto out_pci_suspend;
 
 	netif_device_detach(dev);
-	netif_stop_queue(dev);
+	netif_poll_disable(dev);
 
 	spin_lock_irq(&tp->lock);
 
@@ -3054,6 +3054,7 @@ static int rtl8169_resume(struct pci_dev *pdev)
 	if (!netif_running(dev))
 		goto out;
 
+	netif_poll_enable(dev);
 	netif_device_attach(dev);
 
 	rtl8169_schedule_work(dev, rtl8169_reset_task);
-- 
1.5.2.4

Re: [PATCH] Disable polling in rtl8169_suspend() to prevent race with rtl8169_interrupt()

[Karsten Wiese]

Am Dienstag, 16. Oktober 2007 schrieb Karsten Wiese:
> Hi,
> 
> Sometimes (~ 1 in 10) after suspend to disk my r8169 nic doesn't work and a
> "r8169: eth0: link down" gets mingled up with those suspend messages.
> First tests show nothing bad, so maybe this patch really helps.
> Will post again, if it doesn't.
> 
>       Karsten
> ------------------>
> 
> Disable polling in rtl8169_suspend() to prevent race with rtl8169_interrupt()
> 
> rtl8169_interrupt() is called during polling.
> Without patch, rtl8169_interrupt() could run after rtl8169_asic_down(),
> causing a disabled link state being saved later in rtl8169_suspend() by
> pci_save_state().
> netif_stop_queue() can go as it is called by netif_device_detach().
> 
> Signed-off-by: Karsten Wiese <fzu@wemgehoertderstaat.de>

Forgot to say: patch is against 2.6.23. Stable material, if it gets an ACK.
The BUG happens since months here, if not since I hibernate this pc.

git HEAD would need a similar fix I think, haven't tried yet.

      Karsten

Re: [PATCH] Disable polling in rtl8169_suspend() to prevent race with rtl8169_interrupt()

[Francois Romieu]

Karsten Wiese <fzu@wemgehoertderstaat.de> :
[...]
> Forgot to say: patch is against 2.6.23. Stable material, if it gets an ACK.
> The BUG happens since months here, if not since I hibernate this pc.

Can you give the patch below a try too ?


diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 285b2b2..fcd79b7 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -2424,6 +2424,7 @@ static void rtl8169_reset_task(struct work_struct *work)
 		rtl8169_init_ring_indexes(tp);
 		rtl_hw_start(dev);
 		netif_wake_queue(dev);
+		rtl8169_check_link_status(dev, tp, tp->mmio_addr);
 	} else {
 		if (net_ratelimit() && netif_msg_intr(tp)) {
 			printk(PFX KERN_EMERG "%s: Rx buffers shortage\n",
-- 
Ueimor

Re: [PATCH] Disable polling in rtl8169_suspend() to prevent race with rtl8169_interrupt()

[Karsten Wiese]

Am Dienstag, 16. Oktober 2007 schrieb Francois Romieu:
> Karsten Wiese <fzu@wemgehoertderstaat.de> :
> [...]
> > Forgot to say: patch is against 2.6.23. Stable material, if it gets an ACK.
> > The BUG happens since months here, if not since I hibernate this pc.
> 
> Can you give the patch below a try too ?

I don't know how to reliably make the bug happen, it occured once a week
"normally". Before I try please look through this stuff:

with an unpatched r8169 after bug happened today morning i gathered some
diagnostics:
  [root@a64 ~]# ethtool eth0
  Settings for eth0:
	Supported ports: [ TP ]
	Supported link modes:   10baseT/Half 10baseT/Full 
	                        100baseT/Half 100baseT/Full 
	                        1000baseT/Full 
	Supports auto-negotiation: Yes
	Advertised link modes:  10baseT/Half 10baseT/Full 
	                        100baseT/Half 100baseT/Full 
	                        1000baseT/Full 
	Advertised auto-negotiation: Yes
	Speed: 100Mb/s
	Duplex: Full
	Port: Twisted Pair
this          ^..........^ looked funny to me 
	PHYAD: 0
	Transceiver: internal
	Auto-negotiation: on
	Supports Wake-on: pumbg
	Wake-on: g
	Current message level: 0x00000033 (51)
	Link detected: no
unexpected too         ^^

then i did:
  [root@a64 ~]# ethtool -s eth0 port mii
  [root@a64 ~]# ethtool eth0
  Settings for eth0:
        Supported ports: [ TP ]
        Supported link modes:   10baseT/Half 10baseT/Full
                                100baseT/Half 100baseT/Full
                                1000baseT/Full
        Supports auto-negotiation: Yes
        Advertised link modes:  10baseT/Half 10baseT/Full
                                100baseT/Half 100baseT/Full
                                1000baseT/Full
        Advertised auto-negotiation: Yes
        Speed: 100Mb/s
        Duplex: Full
        Port: Twisted Pair
back to normal ^.........^
        PHYAD: 0
        Transceiver: internal
        Auto-negotiation: on
        Supports Wake-on: pumbg
        Wake-on: g
        Current message level: 0x00000033 (51)
        Link detected: yes
expected value here too^.^ and eth0 worked again.

Can your patch cause the same effect as the "ethtool -s eth0 port mii" in
the bug case?

Do you agree that rtl8169_interrupt() should not be called after
rtl8169_asic_down() when in rtl8169_suspend() and before rtl8169_resume()?
If yes, how should that be achieved?

      Karsten

Re: [PATCH] Disable polling in rtl8169_suspend() to prevent race with rtl8169_interrupt()

[Karsten Wiese]

Am Mittwoch, 17. Oktober 2007 schrieb Karsten Wiese:

to nonsense comments, please ignore:
> this          ^..........^ looked funny to me
and
> back to normal ^.........^

Re: [PATCH] Disable polling in rtl8169_suspend() to prevent race with rtl8169_interrupt()

[Francois Romieu]

Karsten Wiese <fzu@wemgehoertderstaat.de> :
[...]
> Can your patch cause the same effect as the "ethtool -s eth0 port mii" in
> the bug case?
> 
> Do you agree that rtl8169_interrupt() should not be called after
> rtl8169_asic_down() when in rtl8169_suspend() and before rtl8169_resume()?

I have not thought about it too deeply.

I'd rather say that the driver must be able to handle it. Spurious/shared
irq happen.

-- 
Ueimor