e1000: Don't enable polling in open() (was: e1000: assertion hit in e1000_clean(), kernel 2.6.21.1)

PATCH Applied Auke Kok 7 participants 16 messages May 21, 2007 – May 24, 2007

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Auke Kok

author

2007-05-21 21:51 UTC #0 lore raw

PATCH 1/1 e1000: Don't enable polling in open() (was: e1000: assertion hit in e1000_clean(), kernel 2.6.21.1)

Herbert Xu wrote:
"netif_poll_enable can only be called if you've previously called netif_poll_disable. Otherwise a poll might already be in action and you may get a crash like this."

Removing the call to netif_poll_enable in e1000_open should fix this issue, the only other call to netif_poll_enable is in e1000_up() which is only reached after a device reset or resume.

Bugzilla: http://bugzilla.kernel.org/show_bug.cgi?id=8455
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=240339

Tested by Doug Chapman <[email protected]>

Signed-off-by Auke Kok <[email protected]>

Acked-by Herbert Xu <[email protected]>

---

 drivers/net/e1000/e1000_main.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

drivers/net/e1000/e1000_main.c +0 -4

index 49be393..cbc7feb 100644
@@ -1431,10 +1431,6 @@ e1000_open(struct net_device *netdev)
 	/* From here on the code is the same as e1000_up() */
 	clear_bit(__E1000_DOWN, &adapter->flags);
 
-#ifdef CONFIG_E1000_NAPI
-	netif_poll_enable(netdev);
-#endif
-
 	e1000_irq_enable(adapter);
 
 	/* fire a link status change interrupt to start the watchdog */

Dave Jones

2007-05-21 22:16 UTC #1 lore raw

Quoted 11 lines · Auke Kok

Herbert Xu wrote:
"netif_poll_enable can only be called if you've previously called netif_poll_disable. Otherwise a poll might already be in action and you may get a crash like this."
Removing the call to netif_poll_enable in e1000_open should fix this issue, the only other call to netif_poll_enable is in e1000_up() which is only reached after a device reset or resume.
Bugzilla: http://bugzilla.kernel.org/show_bug.cgi?id=8455
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=240339

This probably doesn't solve the latter bug.
The code you reference isn't there in the kernel tested in that bug
(2.6.21) In 2.6.21, netif_poll_enable is only called from
e1000_up(), not e1000_open()

Dave

Signature · 7 lines

http://www.codemonkey.org.uk

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Herbert Xu

2007-05-22 00:49 UTC #2 lore raw

Quoted 10 lines · Dave Jones

Quoted 9 lines · Auke Kok
Herbert Xu wrote:
"netif_poll_enable can only be called if you've previously called netif_poll_disable. Otherwise a poll might already be in action and you may get a crash like this."
Removing the call to netif_poll_enable in e1000_open should fix this issue, the only other call to netif_poll_enable is in e1000_up() which is only reached after a device reset or resume.

Quoted 7 lines

Quoted 2 lines
Bugzilla: http://bugzilla.kernel.org/show_bug.cgi?id=8455
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=240339
This probably doesn't solve the latter bug.
The code you reference isn't there in the kernel tested in that bug
(2.6.21) In 2.6.21, netif_poll_enable is only called from
e1000_up(), not e1000_open()

Yes we need a different fix for 2.6.21. There e1000_open calls e1000_up which is why we still get the netif_poll_enable.

Cheers,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Kok, Auke

author

2007-05-22 00:58 UTC #3 lore raw

Quoted 14 lines · Herbert Xu

Quoted 13 lines · Dave Jones
Quoted 11 lines · Auke Kok
Herbert Xu wrote:
"netif_poll_enable can only be called if you've previously called netif_poll_disable. Otherwise a poll might already be in action and you may get a crash like this."
Removing the call to netif_poll_enable in e1000_open should fix this issue, the only other call to netif_poll_enable is in e1000_up() which is only reached after a device reset or resume.
Bugzilla: http://bugzilla.kernel.org/show_bug.cgi?id=8455
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=240339

Quoted 7 lines

Quoted 4 lines
This probably doesn't solve the latter bug.
The code you reference isn't there in the kernel tested in that bug
(2.6.21) In 2.6.21, netif_poll_enable is only called from
e1000_up(), not e1000_open()
Yes we need a different fix for 2.6.21. There e1000_open calls e1000_up which is why we still get the netif_poll_enable.

yes, basically they need the patch that introduced(exposed) the problem as well, but that is a rather significant change and kind of moves the whole netstack-init code in e1000 around. The size was the reason why that patch didn't go into 2.6.21 in the first place, but perhaps they can pull both patches into the FC tree.

For reference, this is the commit:

commit e0aac5a289b1dacbc94bd9ae8c449bcdf9ab508c

Author: Auke Kok <[email protected]>
Date:   Tue Mar 6 08:57:21 2007 -0800

e1000: FIX: be ready for incoming irq at pci_request_irq

DEBUG_SHIRQ code exposed that e1000 was not ready for incoming interrupts after having called pci_request_irq. This obviously requires us to finish our software setup which assigns the irq handler before we request the irq.

Signed-off-by Auke Kok <[email protected]>

Signed-off-by Jeff Garzik <[email protected]>

Dave, would that be an option for you?

Auke

Dave Jones

2007-05-22 01:22 UTC #4 lore raw

Quoted 27 lines · Auke

Quoted 7 lines
Quoted 4 lines
This probably doesn't solve the latter bug.
The code you reference isn't there in the kernel tested in that bug
(2.6.21) In 2.6.21, netif_poll_enable is only called from
e1000_up(), not e1000_open()
Yes we need a different fix for 2.6.21. There e1000_open calls e1000_up which is why we still get the netif_poll_enable.
yes, basically they need the patch that introduced(exposed) the problem as well, but that is a rather significant change and kind of moves the whole netstack-init code in e1000 around. The size was the reason why that patch didn't go into 2.6.21 in the first place, but perhaps they can pull both patches into the FC tree.
For reference, this is the commit:
commit e0aac5a289b1dacbc94bd9ae8c449bcdf9ab508c
Author: Auke Kok <[email protected]>
Date:   Tue Mar 6 08:57:21 2007 -0800
e1000: FIX: be ready for incoming irq at pci_request_irq
DEBUG_SHIRQ code exposed that e1000 was not ready for incoming interrupts after having called pci_request_irq. This obviously requires us to finish our software setup which assigns the irq handler before we request the irq.

Quoted 4 lines

Signed-off-by Auke Kok <[email protected]>
Signed-off-by Jeff Garzik <[email protected]>
Dave, would that be an option for you?

Sounds like a plan. I'll do a test-build with this and the other patch, and throw it at the people seeing the problem tomorrow.

Thanks,

Dave

Signature · 7 lines

http://www.codemonkey.org.uk

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Jeff Garzik

Applied

2007-05-21 23:42 UTC #5 lore raw

In reply to Auke Kok

Quoted 12 lines · Auke Kok

Herbert Xu wrote:
"netif_poll_enable can only be called if you've previously called netif_poll_disable. Otherwise a poll might already be in action and you may get a crash like this."
Removing the call to netif_poll_enable in e1000_open should fix this issue, the only other call to netif_poll_enable is in e1000_up() which is only reached after a device reset or resume.
Bugzilla: http://bugzilla.kernel.org/show_bug.cgi?id=8455
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=240339

Quoted 8 lines

Tested by Doug Chapman <[email protected]>
Signed-off-by Auke Kok <[email protected]>
Acked-by Herbert Xu <[email protected]>
---
 drivers/net/e1000/e1000_main.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

applied, though as a poster (DaveJ?) noted, I'm not sure it completely fixes the bug

Kok, Auke

author

2007-05-22 00:04 UTC #6 lore raw

Quoted 19 lines · Jeff Garzik

Quoted 18 lines · Auke Kok
Herbert Xu wrote:
"netif_poll_enable can only be called if you've previously called netif_poll_disable. Otherwise a poll might already be in action and you may get a crash like this."
Removing the call to netif_poll_enable in e1000_open should fix this issue, the only other call to netif_poll_enable is in e1000_up() which is only reached after a device reset or resume.
Bugzilla: http://bugzilla.kernel.org/show_bug.cgi?id=8455
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=240339
Tested by Doug Chapman <[email protected]>
Signed-off-by Auke Kok <[email protected]>
Acked-by Herbert Xu <[email protected]>
---

Quoted 5 lines

Quoted 2 lines
 drivers/net/e1000/e1000_main.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)
applied, though as a poster (DaveJ?) noted, I'm not sure it completely fixes the bug

It fixes the HP IA64 issue reported by Doug Chapman, but indeed does not even apply to the FC bugzilla. I was mistaken due to the fact that I sent the code that introduced the bug a while ago, but it's not in 2.6.21 (it was in the e1000-fixes tree you had and merged after 2.6.21).

So, this patch doesn't apply for -stable.

Auke

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Herbert Xu

2007-05-22 01:01 UTC #7 lore raw

In reply to Jeff Garzik

Quoted 3 lines · Jeff Garzik

applied, though as a poster (DaveJ?) noted, I'm not sure it completely fixes the bug

It should fix the problem completely in 2.6.22. For 2.6.21, we need a different fix because e1000_open is directly calling e1000_up.

Cheers,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Chuck Ebbert

2007-05-22 16:56 UTC #8 lore raw

Quoted 7 lines · Herbert Xu

Quoted 2 lines · Jeff Garzik
applied, though as a poster (DaveJ?) noted, I'm not sure it completely fixes the bug
It should fix the problem completely in 2.6.22. For 2.6.21, we need a different fix because e1000_open is directly calling e1000_up.

Is there going to be a 2.6.21-stable fix for this?

Fedora is going to backport a big patchset but that won't work for -stable.

Kok, Auke

author

2007-05-22 17:02 UTC #9 lore raw

Quoted 4 lines · Chuck Ebbert

Quoted 3 lines · Herbert Xu
Quoted 2 lines · Jeff Garzik
applied, though as a poster (DaveJ?) noted, I'm not sure it completely fixes the bug

Quoted 8 lines

Quoted 3 lines
It should fix the problem completely in 2.6.22. For 2.6.21, we need a different fix because e1000_open is directly calling e1000_up.
Is there going to be a 2.6.21-stable fix for this?
Fedora is going to backport a big patchset but that won't work for -stable.

I've posted that before and it's up to the stable team. I think that it's a seriously toolarge change unless Herbert posts his short version of the fix for 2.6.21.1. I would be OK with that.

BTW this bug is present in most recent kernels, certainly before 2.6.20...

Auke

Herbert Xu

2007-05-23 10:47 UTC #10 lore raw

Quoted 4 lines · Auke

I've posted that before and it's up to the stable team. I think that it's a seriously toolarge change unless Herbert posts his short version of the fix for 2.6.21.1. I would be OK with that.

This patch should work for 2.6.21.

Cheers,

Signature · 5 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--

drivers/net/e1000/e1000_main.c +1 -0

index b28a915..1e4e722 100644
@@ -1423,6 +1423,7 @@ e1000_open(struct net_device *netdev)
 
 	e1000_power_up_phy(adapter);
 
+	netif_poll_disable(netdev);
 	if ((err = e1000_up(adapter)))
 		goto err_up;
 	adapter->mng_vlan_id = E1000_MNG_VLAN_NONE;

Chris Wright

2007-05-23 23:34 UTC #11 lore raw

Quoted 7 lines · * Herbert Xu ([email protected]) wrote:

Quoted 4 lines · Auke
I've posted that before and it's up to the stable team. I think that it's a seriously toolarge change unless Herbert posts his short version of the fix for 2.6.21.1. I would be OK with that.
This patch should work for 2.6.21.

Mind adding an updated changelog and signed-off-by please?

thanks,
-chris

Kok, Auke

author

2007-05-23 23:38 UTC #12 lore raw

Quoted 6 lines · Chris Wright

Quoted 3 lines · * Herbert Xu ([email protected]) wrote:
Quoted 1 line · Auke
I've posted that before and it's up to the stable team. I think that it's a seriously toolarge change unless Herbert posts his short version of the fix for 2.6.21.1. I would be OK with that.
This patch should work for 2.6.21.
Mind adding an updated changelog and signed-off-by please?

Herbert,

With that, please add my Acked-By...

Auke

Herbert Xu

2007-05-24 01:29 UTC #13 lore raw

Quoted 2 lines · Auke

With that, please add my Acked-By...

Sorry, I only read yours after Chris's message :)

Chris, please add that Acked-By by Auke.

Cheers,

Signature · 10 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Chris Wright

2007-05-24 04:31 UTC #14 lore raw

Quoted 7 lines · * Herbert Xu ([email protected]) wrote:

Quoted 2 lines · Auke
With that, please add my Acked-By...
Sorry, I only read yours after Chris's message :)
Chris, please add that Acked-By by Auke.

Got it, thanks.
-chris

Herbert Xu

2007-05-24 01:29 UTC #15 lore raw

In reply to Chris Wright

Quoted 2 lines · Chris Wright

Mind adding an updated changelog and signed-off-by please?

Sure.

[E1000]: Fix netif_poll_enable crash in e1000_open

e1000_open calls e1000_up which will always call netif_poll_enable. However, it's illegal to call netif_poll_enable without calling netif_poll_disable at some prior time. This constraint is violated in e1000_open.

This patch fixes it by simply calling netif_poll_disable in e1000_open.

Signed-off-by Herbert Xu <[email protected]>

Cheers,

Signature · 5 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--

drivers/net/e1000/e1000_main.c +1 -0

index b28a915..1e4e722 100644
@@ -1423,6 +1423,7 @@ e1000_open(struct net_device *netdev)
 
 	e1000_power_up_phy(adapter);
 
+	netif_poll_disable(netdev);
 	if ((err = e1000_up(adapter)))
 		goto err_up;
 	adapter->mng_vlan_id = E1000_MNG_VLAN_NONE;