Patchwork 8139cp: re-enable interrupts after tx timeout

login
register
mail settings
Submitter David Woodhouse
Date Nov. 24, 2012, 10:11 p.m.
Message ID <1353795081.26346.287.camel@shinybook.infradead.org>
Download mbox | patch
Permalink /patch/201479/
State Accepted
Delegated to: David Miller
Headers show

Comments

David Woodhouse - Nov. 24, 2012, 10:11 p.m.
Recovery doesn't work too well if we leave interrupts disabled...

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
For stable too, I suppose.
fran├žois romieu - Nov. 24, 2012, 10:58 p.m.
David Woodhouse <dwmw2@infradead.org> :
> Recovery doesn't work too well if we leave interrupts disabled...
> 
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

Acked-by: Francois Romieu <romieu@fr.zoreil.com>

While you are at it, do you see what would prevent the watchdog
timer and the rx napi handler to race on two different CPUs
("using a Geode" could be an answer :o) ) ?

> ---
> For stable too, I suppose.

Yes. Watch for http://patchwork.ozlabs.org/bundle/davem/stable/
David Woodhouse - Nov. 24, 2012, 11:27 p.m.
On Sat, 2012-11-24 at 23:58 +0100, Francois Romieu wrote:
> While you are at it, do you see what would prevent the watchdog
> timer and the rx napi handler to race on two different CPUs

Hm, good point. Is it sufficient just to stick napi_disable() and
napi_enable() in there? And should we try to reset the TX state machine
without stomping on RX at all?
David Miller - Nov. 25, 2012, 8:57 p.m.
From: Francois Romieu <romieu@fr.zoreil.com>
Date: Sat, 24 Nov 2012 23:58:55 +0100

> David Woodhouse <dwmw2@infradead.org> :
>> Recovery doesn't work too well if we leave interrupts disabled...
>> 
>> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> 
> Acked-by: Francois Romieu <romieu@fr.zoreil.com>

Applied to net-next
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
fran├žois romieu - Nov. 25, 2012, 10:43 p.m.
David Woodhouse <dwmw2@infradead.org> :
> On Sat, 2012-11-24 at 23:58 +0100, Francois Romieu wrote:
> > While you are at it, do you see what would prevent the watchdog
> > timer and the rx napi handler to race on two different CPUs
> 
> Hm, good point. Is it sufficient just to stick napi_disable() and
> napi_enable() in there?

No, napi_disable() may sleep.

> And should we try to reset the TX state machine without stomping on
> RX at all ?

Either that or the watchdog timer will have to queue some user context
work. Things may change later when you go lockless (hint, hint).

Patch

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 1c81825..ad6afc3 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -1192,6 +1192,7 @@  static void cp_tx_timeout(struct net_device *dev)
 	cp_clean_rings(cp);
 	rc = cp_init_rings(cp);
 	cp_start_hw(cp);
+	cp_enable_irq(cp);
 
 	netif_wake_queue(dev);